Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernization of lib? #166

Closed
robertleeplummerjr opened this issue Aug 23, 2019 · 11 comments
Closed

Modernization of lib? #166

robertleeplummerjr opened this issue Aug 23, 2019 · 11 comments

Comments

@robertleeplummerjr
Copy link
Collaborator

I'm curious what thoughts are on moving constructors into their own files, as classes, some cleanup, and adding semicolons?

@robertleeplummerjr
Copy link
Collaborator Author

My IDE (WebStorm) goes into HIGH cpu mode when I open the lib because of no semicolons, is why I bring it up.

@dhritzkiv
Copy link
Member

lol sounds like a bad IDE…

@dhritzkiv
Copy link
Member

But yes, the library could use some modernization in terms of cleaning up into classes, separate files, etc. and adding better style formatting + linting.

@robertleeplummerjr
Copy link
Collaborator Author

I started looking into a bottleneck I was having, and took a step back and saw we're making fairly heavy use of fn.call(), to sort of shim in class like behavior. I think in most places we're fine doing this, but when researching if there is a performance penalty doing this, especially for resource intense areas, it seems there can be some pretty large performance implications. I just ran this https://jsperf.com/function-versus-function-call-versus-function-apply on chrome and here were the results:
Screen Shot 2019-08-27 at 10 05 57 AM

In some cases, the compiler can optimize the code, especially if it is highly repetitive, but it still warrants some attention, I think.

Screen Shot 2019-08-27 at 10 06 07 AM

Yes, this is just chrome, but by implication, it is node. Here are firefox's results, just for comparison:
Screen Shot 2019-08-27 at 10 06 32 AM

I'd be surprised, even with the millions of operations per second, if we actually could see much of a change, but because it is of good practice, I've started taking a stab at translating what we have currently over to a full class structure, with super.method() calls. It should give us a strategic base for perhaps starting on webgl2, but will simply give us a clean start for newer node, and keeping this package up to date, and performant.

@robertleeplummerjr
Copy link
Collaborator Author

Behold! Classes! - f5139a2

@dhritzkiv
Copy link
Member

Noice. Thanks for cleaning up the directories/structure of the project, too

@robertleeplummerjr
Copy link
Collaborator Author

I've got most of the library working fine, but for some reason when I get to the test where it is failing in travis I've narrowed it down to the context.destroy() which calls the super.destroy() works the first time, but the second call causes node to throw a pretty ambiguous exception: Abort trap: 6, any clues?

@dhritzkiv
Copy link
Member

No. Some cursory searching for the term suggests we're writing to memory we don't own? I'm not sure how that's possible.

Could be a Travis issue

@robertleeplummerjr
Copy link
Collaborator Author

I miss-read the travis failure, it is legit, however the issue I was seeing, though near that failing unit test, actually seems to come from this line:

        assertOk("x < 0 full width", function() {
            gl.copyTexSubImage2D(gl.TEXTURE_2D, 0, 0, 0, -1, 0, 16, 16);
        });

Later, when the context is being disposed, it throws the Abort trap: 6 in c++, and causes node to die. I believe this is actually an OS or driver issue. Similar bug in firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1176404. Oh well. Commented it out, and progressing through other meaningful broken parts, will have them fixed as time allots.

@robertleeplummerjr
Copy link
Collaborator Author

Pushed some changes that fixed nearly all unit test breaks. Maybe tomorrow or Tuesday we'll be near or at a pull request. Just fyi.

@robertleeplummerjr
Copy link
Collaborator Author

PR opened #168

dhritzkiv added a commit that referenced this issue Sep 9, 2019
…ufferStatus-high-cpu

FIX #166 const let and precheck framebuffer status high cpu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants