Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Apr 30, 2020

  • mayHaveStringValue used a recursion pattern through a super-call which the optimizer isn't handling very well. Switched to a more direct style.
  • The above caused a bad join order in AmdModuleDefinition.getFactoryNode(), which can be avoided by caching AMD modules.
  • It also caused bad join orders for uses of PropWrite.getRhs(), generally by putting TValueNode first in the join order. It turns out getRhs was actually recursive with hasPropertyWrite which I decided to fix, after which magic made it recursive again, which I fixed by adding pragma[nomagic] in a few key places.

Evaluations all show a speed-up of a few percent:

Based on some quick tuple counting, the second commit, and third+fourth commit, independently cause a speed-up, whereas the first commit is dependent on the others.

@asgerf asgerf added the JS label Apr 30, 2020
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the mayHaveStringValue change 👍

I don't like using cached to fix a join-order, but I assume you explored other options.

The change in ObjectDefinePropertyAsPropWrite doesn't make the code prettier, but I get why it can make is faster.
We loose support for reflective calls to Object.defineProperty with this PR, but I think that is acceptable.

Nice speedup 🎉
LGTM 👍

@semmle-qlci semmle-qlci merged commit c66ec3c into github:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants