Skip to content

Implement SAFE_HEAP using acorn, and use it on user JS - #12450

Merged
kripken merged 24 commits into
masterfrom
safeHeap
Oct 7, 2020
Merged

kripken merged 24 commits into
masterfrom
safeHeap

Conversation

@kripken

@kripken kripken commented Oct 6, 2020

Copy link
Copy Markdown
Member

Previously we instrumented the compiled code, and also code in JS libraries
using {{{ makeGetValue }}} etc., but not arbitrary HEAP8 etc. usage in user
JS code. This fixed that by adding a safeHeap pass to the acorn optimizer.

This found a few issues in our JS code too, which have been fixed in
recent PRs already.

@sbc100 sbc100 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we still need the JS library technique of using {{{ makeGetValue }}} or can that now be removed in favor of this?

Comment thread tests/test_other.py Outdated

def test_js_optimizer(self):
ACORN_PASSES = ['JSDCE', 'AJSDCE', 'applyImportAndExportNameChanges', 'emitDCEGraph', 'applyDCEGraphRemovals', 'growableHeap', 'unsignPointers', 'asanify']
ACORN_PASSES = ['JSDCE', 'AJSDCE', 'applyImportAndExportNameChanges', 'emitDCEGraph', 'applyDCEGraphRemovals', 'growableHeap', 'unsignPointers', 'asanify', 'safeHeap']

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Time to split this one one-element-per-line, or at least multiple lines?

@kripken

kripken commented Oct 7, 2020

Copy link
Copy Markdown
Member Author

Do we still need the JS library technique of using {{{ makeGetValue }}} or can that now be removed in favor of this?

We can maybe remove it, yeah. There is some advantage to the makeGetValue notation, in that you don't need to write the >> 2 etc., which can be error prone. But I agree it's probably not worth it given the verbosity, overall it's less clear. If you agree I can look into it.

@kripken
kripken merged commit bec573b into master Oct 7, 2020
@kripken
kripken deleted the safeHeap branch October 7, 2020 18:41
@sbc100

sbc100 commented Oct 7, 2020

Copy link
Copy Markdown
Collaborator

Do we still need the JS library technique of using {{{ makeGetValue }}} or can that now be removed in favor of this?

We can maybe remove it, yeah. There is some advantage to the makeGetValue notation, in that you don't need to write the >> 2 etc., which can be error prone. But I agree it's probably not worth it given the verbosity, overall it's less clear. If you agree I can look into it.

I wasn't suggesting removing {{{ makeGetValue }}} just that it should not try do any SAFE_HEAP stuff.. then the SAFE_HEAP stuff is all in one place and will be consistent between user code and library code?

@kripken

kripken commented Oct 7, 2020

Copy link
Copy Markdown
Member Author

Oh, yes, it is redundant atm. Rather than remove it, though, I'd prefer to eventually remove {{{ makeGetValue }}} entirely.

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

Successfully merging this pull request may close these issues.

2 participants