[Merged by Bors] - Implement the global eval() function
#2041
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
raskad
added
enhancement
Codecov Report
@@ Coverage Diff @@
## main #2041 +/- ##
==========================================
- Coverage 44.16% 43.82% -0.35%
==========================================
Files 212 213 +1
Lines 18812 19161 +349
==========================================
+ Hits 8309 8397 +88
- Misses 10503 10764 +261
Continue to review full report at Codecov.
|
Benchmark for bde8e49Click to view benchmark
|
VM implementation
Fixed tests (1364):Broken tests (26): |
Test262 conformance changesVM implementation
Fixed tests (1170):Broken tests (24): |
raskad
requested review from
Razican,
HalidOdat,
jedel1043,
jasonwilliams and
RageKnify
Benchmark for 7e1ca2aClick to view benchmark
|
Razican
reviewed
Apr 28, 2022
There was a problem hiding this comment.
Looks pretty good!! Check my comments to see if I found some stuff to improve.
Do you think it would be possible to implement the evalScript() function of the $262 object with this? https://github.com/tc39/test262/blob/main/INTERPRETING.md#host-defined-functions
boa_engine/src/builtins/eval/mod.rs
Outdated
| Self::perform_eval(args.get_or_undefined(0), false, false, context) | ||
| } | ||
|
|
||
| /// `19.2.1.1 PerformEval ( x, callerRealm, strictCaller, direct )` |
There was a problem hiding this comment.
As far as I can tell, the spec says this is:
19.2.1.1 PerformEval ( x, strictCaller, direct )
No mention of the callerRealm there, right?
There was a problem hiding this comment.
True, maybe it changed since I copied it
There was a problem hiding this comment.
Yes, it changed a few days ago: tc39/ecma262#2670
| strict: bool, | ||
| context: &mut Context, | ||
| ) -> Result<JsValue, JsValue> { | ||
| let x = if let Some(x) = x.as_string() { |
There was a problem hiding this comment.
Could we add the comments relating to the spec directly where possible? It makes it much easier to follow the spec and maintain it in the future.
In any case, I see that the spec has been re-interpreted, not following the strict order as mentioned there, probably due to implementation reasons, but some comments here to explain the logic would be helpful.
There was a problem hiding this comment.
Yeah I will try to add some comments and maybe match them to some of the spec comments. You're right, the implementation details diverge very hard from the spec here.
boa_engine/src/builtins/eval/mod.rs
Outdated
| return Ok(x.clone()); | ||
| }; | ||
|
|
||
| let parsing_result = context.parse(x.as_bytes()).map_err(|e| e.to_string()); |
There was a problem hiding this comment.
I think this would avoid one layer of indirection, but not sure if it can be done like this.
Suggested change
| let parsing_result = context.parse(x.as_bytes()).map_err(|e| e.to_string()); | |
| let parsing_result = context.parse(x.as_bytes()).map_err(ToString::to_string); |
There was a problem hiding this comment.
I don't think this is possible. map_err expects the closure to take E by value but ToString::to_string takes the input by reference. I would expect the compiler to optimize the additional closure away.
Comment on lines
+20
to
+25
| /// Checking all environments for potential added bindings at runtime on every get/set | ||
| /// would offset the performance improvement of determining binding locations at compile time. | ||
| /// To minimize this, each environment holds a `poisoned` flag. | ||
| /// If bindings where added at runtime, the current environment and all inner environments | ||
| /// are marked as poisoned. | ||
| /// All poisoned environments have to be checked for added bindings. |
There was a problem hiding this comment.
I like this, is probably the only way to avoid a huge performance penalty
There was a problem hiding this comment.
I think so too. There may be some minor changes that could bring some performance benefits, but I think in principle this is the best that can be done. I have not looked at v8 or spidermonkey, but my guess is they do something very similar. Interestingly enough I found the exact description of the performance problem on mdn after I did the implementation :D [...] any use of eval() will force the browser to do long expensive variable name lookups to figure out where the variable exists [...]
Benchmark for c5bb910Click to view benchmark
|
HalidOdat
approved these changes
May 9, 2022
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 9, 2022
This Pull Request fixes/closes #948. It changes the following: - Implement the global `eval()` function. Runtime code evaluation brings some challenges for environments. Currently the setting and getting of variable bindings is done via indices that are calculated at compile time. This prevents costly hashmap lookups at runtime. Evaluiation at runtime needs access to existing compile time environments. This is a relatively easy change. We wrap compile time environments in `Gc` and make them accessible at runtime. Because `eval()` can add var bindings to existing function environments, we have to adjust the environments for this. Because we cannot recompile all previously stored binding indices, we have to fallback to hashmap lookups at runtime. To prevent this from tanking our performance we add a flag to each environment that marks if any `eval()` has been executed in that environment (or outer environments). This makes it possible to retain the performance of precompiled environment lookups while having a fallback for `eval()`. TLDR: `eval()` is not only horribly unsafe but also a burden for performance. [Never use eval()!](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#never_use_eval!)
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
eval() functioneval() function
Razican
pushed a commit
that referenced
this pull request
Jun 8, 2022
This Pull Request fixes/closes #948. It changes the following: - Implement the global `eval()` function. Runtime code evaluation brings some challenges for environments. Currently the setting and getting of variable bindings is done via indices that are calculated at compile time. This prevents costly hashmap lookups at runtime. Evaluiation at runtime needs access to existing compile time environments. This is a relatively easy change. We wrap compile time environments in `Gc` and make them accessible at runtime. Because `eval()` can add var bindings to existing function environments, we have to adjust the environments for this. Because we cannot recompile all previously stored binding indices, we have to fallback to hashmap lookups at runtime. To prevent this from tanking our performance we add a flag to each environment that marks if any `eval()` has been executed in that environment (or outer environments). This makes it possible to retain the performance of precompiled environment lookups while having a fallback for `eval()`. TLDR: `eval()` is not only horribly unsafe but also a burden for performance. [Never use eval()!](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#never_use_eval!)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request fixes/closes #948.
It changes the following:
eval()function.Runtime code evaluation brings some challenges for environments. Currently the setting and getting of variable bindings is done via indices that are calculated at compile time. This prevents costly hashmap lookups at runtime.
Evaluiation at runtime needs access to existing compile time environments. This is a relatively easy change. We wrap compile time environments in
Gcand make them accessible at runtime.Because
eval()can add var bindings to existing function environments, we have to adjust the environments for this. Because we cannot recompile all previously stored binding indices, we have to fallback to hashmap lookups at runtime. To prevent this from tanking our performance we add a flag to each environment that marks if anyeval()has been executed in that environment (or outer environments). This makes it possible to retain the performance of precompiled environment lookups while having a fallback foreval().TLDR:
eval()is not only horribly unsafe but also a burden for performance. Never use eval()!