Thank you!

Thank you!
The approach looks good and less error-prone!

Thank you!

Thank you for testing and updating!

I have questions for hash2 calculation.

Thank you for updating!
Here's try run
https://treeherder.mozilla.org/jobs?repo=try&revision=c5626976610bd6634080e6dbaeaf9f1b5a2dfb52

The try run shows some failures.
Some of them (e.g. bc) seem to be infra issue, but the failure in devtools tests may be real.
https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=3c06576cc74ea12209824be25ac5eaeab8fa2ad0&selectedTaskRun=Iem4w0RqSeW2G_52zw3yGg.0

here's try run https://treeherder.mozilla.org/jobs?repo=try&revision=3c06576cc74ea12209824be25ac5eaeab8fa2ad0

Looks good! Thank you!

Thank you for updating!
It's almost there :)

Looks good! Thank you for working on improving the API design!

Thank you for updating.
There are some bugs in when to invalidate the JIT code, and also how the redeclaration between different lexical binding kind works.

Still partial review, but there's a critical bug where the mutation is not reflected to JIT-compiled code.

Thank you!

In D326431#11367413, @nchevobbe wrote:

In D326431#11367328, @arai wrote:

replMode name needs to be renamed to reflect the actual behavior.
Such as, allowRedeclaringExistingLexicalBinding or something. Otherwise, it's hard to figure out the intention and the effect in each occurrence of replMode name.

I think I was the one suggesting such name because we want to have other niceties for such "mode" (e.g. be able to evaluate top-level await expression)
But I guess we could have many options with explicit names rather than only one covering everything

replMode name needs to be renamed to reflect the actual behavior.
Such as, allowRedeclaringExistingLexicalBinding or something. Otherwise, it's hard to figure out the intention and the effect in each occurrence of replMode name.

The migration to classes looks good, but the naming and the ownership design looks misleading throughout the patch stack.

I'll continue the review tomorrow.

r+ with the TODO comment addressed

Thank you for investigation!

Please remove "WIP:" prefix from the commit message.
it's automatically added when you submit with wip status, and it's not automatically removed if you convert it to "needs review" from the web interface.
You can remove it from "Edit Revision" link in the side bar.

testing-exception-other: we don't have OOM test here

Thank you for updating.
Can you push this to try and post the URL here?

r+ with the comment addressed.

Thank you so much for working on this!
r+ with the comments addressed.

Thank you for updating!
It's almost there :)

In D322930#11228679, @jandem wrote:

I'm not sure about adding the check to AutoCheckRecursionLimit because it seems these are pretty different use cases. It might be simpler and more predictable perf-wise to check the cancel flag at the beginning of each JS function we parse, constant fold, and emit bytecode for, or something? (If we want to be more precise we could change that to the beginning of each scope.)

The fundamental approach looks okay, but we should add several restriction and assertions, and rename some functions/variables to be more descriptive, to ensure that we never misuse it.

r+ with the remaining comments addressed

Thank you again!
Only some small nits.

Thank you for working on this!

testing-exception-elsewhere: D322914

Thank you for addressing the comments.
There are some more small fixes necessary.

I think we don't really need the "shared" buffer system, but we can just pass the sole ownership around, which should be simpler and easier to reason about.

Thank you!

I'm a bit confused by the flag handling for hasTrackedHostDefinedGlobal_ and hasTrackedHostDefinedData_.

I'll review this patch this weekend, or maybe Monday

Nice simplification!

testing-exception-elsewhere: D321815

testing-exception-elsewhere: D321137

Thank you for updating!
Yes, the patch looks good!

Please mark this revision abandoned:

1. Click "Add Action..." drop down menu above the comment field.
2. Select "Abandon Revision"
3. Click "Submit" below the comment field

Thank you for working on this.
Please remove the "Amend: Fix error detected by reviewbot" from the commit message, given that's unrelated to the patch itself.

Here's try run: https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=c3d87c2eb998729952716ae92ad2aaabb0b8af19

Thank you!

Thank you!

The summary and the comment contains unnecessary information.

I'll finish reviewing this and later patches tomorrow

testing-exception-elsewhere: The consumer of the additional code path is added in later patches, and the existing testcases covers them.

r+ with some more comments added.

Looks good!

There are some small nits, and also some questions.

r+ with some fix, and also the verification about the lifetime in this and the later patch.

r+ with the fixes, in this patch and the later patches.

If there's no one who's monitoring this probe, the change itself looks good, but I'm also not familiar with the background.

Thank you!

Thank you!

This patch should be split into 2 or 3 separate patches, for each orthogonal change and semantic chunk, and preparation that's supposed to have no effect,
and each change should be evaluated separately, especially some benchmarks show regressions.

Thank you!

logic-wise, looks good, but the function names and the comment need some fix and clarification.

Can you push this patch stack to the try server, with the following jobs selected, and post the URL here?

• spidermonkey-sm-plain-linux64/opt
• spidermonkey-sm-plain-linux64/debug
• spidermonkey-sm-nonunified-linux64/debug
• hazard-linux64-shell-haz/debug
• test-linux2404-64/debug-web-platform-tests-*