Refactor old function with new function object #255
Merged
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
jasonwilliams
changed the title
This was referenced Feb 20, 2020
Benchmark for bb3f46cClick to view benchmark
|
jasonwilliams
force-pushed
the
function_objs
branch
from
10f4256 to
0c07ddb
Compare
Benchmark for 6f94fafClick to view benchmark
|
|
I think this would be a nice addition to Boa 0.8, and it could potentially solve #266. What extra work is needed here? |
|
I’ll dig this up again, I remember being blocked on something, leave it with me for now |
Merged
jasonwilliams
force-pushed
the
function_objs
branch
from
0c07ddb to
a271e6b
Compare
|
Most of the properties are there, now its a case of replacing the old function with this one |
Benchmark for 4c03ffaClick to view benchmark
|
jasonwilliams
force-pushed
the
function_objs
branch
from
f8bcb25 to
bd0b883
Compare
|
Almost there, i hit a pretty weird error. Would appreciate another pair of eyes function hello() {
console.log("hello");
}
const a = hello();
a; |
|
I will take a look, to see whats causing this. :) |
|
Well... Its in an unsafe block in I don't know much about the Hope that helps. :) |
function hello() {
console.log("hello");
}
const a = hello();
a;The above code runs and returns |
|
Hey thanks @HalidOdat Ill take a look and see what im doing with Gc |
|
Sorted it Tests remaining |
jasonwilliams
changed the title
|
Why do we have |
function will be replaced and function_object will be called function, i just need them both right now while i transition in this PR. once all tests pass and im happy with it ill get rid of all the old function code |
jasonwilliams
force-pushed
the
function_objs
branch
from
8e148cd to
6c0963f
Compare
|
Ugh I’m going to have to refactor this, I forgot a single function can be both a call and construct call. So in the function struct I think Instead of body I’ll need call_body and construct_body |
Razican
changed the title
|
I'm leaning towards the idea of Object having a |
Benchmark for b027db7Click to view benchmark
|
Benchmark for 64452b3Click to view benchmark
|
Benchmark for 19ff051Click to view benchmark
|
Benchmark for c13aef3Click to view benchmark
|
Benchmark for 649cfb5Click to view benchmark
|
Benchmark for b14cceeClick to view benchmark
|
|
Wow, great job! It seems that execution time has gone up by more than 50%. I will try to see if there is any improvement to make in performance. |
|
I did some profiling, and I noticed that our biggest pain point is with our heavy usage of I will re-check the code changes just in case. |
Razican
reviewed
May 2, 2020
| @@ -478,7 +475,7 @@ pub fn sin(_: &Value, args: &[Value], _: &mut Interpreter) -> ResultValue { | |||
| /// | |||
| /// [spec]: https://tc39.es/ecma262/#sec-math.sinh | |||
| /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/sinh | |||
| pub fn sinh(_: &Value, args: &[Value], _: &mut Interpreter) -> ResultValue { | |||
| pub fn sinh(_: &mut Value, args: &[Value], _: &mut Interpreter) -> ResultValue { | |||
There was a problem hiding this comment.
Just for my understanding, why do we need all these to be mutable?
There was a problem hiding this comment.
The first argument this can be altered by Constructor functions.
Its up to them to change the this value however they want.
Example
Usually this starts out being a plain ordinary JS object, and new String() will add a [[StringData]] internal slot to it. This mutates the value.
Why this wasn't a compile error before i don't know, but something in my refactoring meant that rust now moans if you mutate it.
imo it should have always been an issue but we got away with it before.
There was a problem hiding this comment.
For regular functions it doesn't need to be mutable, as the first argument is a reference to the function itself (wrapped in a Value).
I think in future we can have 2 separate signatures for Construct and Func, for now they use the same.
Razican
reviewed
May 2, 2020
Razican
reviewed
May 2, 2020
Razican
reviewed
May 2, 2020
boa/src/builtins/object/mod.rs
Outdated
| } | ||
|
|
||
| /// Defines the different types of objects. | ||
| #[derive(Trace, Finalize, Clone, Debug, Eq, PartialEq)] | ||
| #[derive(Trace, Finalize, Debug, Clone, Eq, PartialEq)] |
There was a problem hiding this comment.
We should probably implement Copy here. For that, we need to do an unsafe implementation of an empty Trace until Manishearth/rust-gc#87 gets resolved.
| /// | ||
| /// In our implementation, Function is extending Object by holding an object field which some extra data | ||
| /// Arrow functions don't define a `this` and thus are lexical, `function`s do define a this and thus are NonLexical | ||
| #[derive(Trace, Finalize, Debug, Clone)] |
There was a problem hiding this comment.
We should implement Copy here, by doing an unsafe implementation of an empty Trace, until Manishearth/rust-gc#87 gets solved.
|
@Razican the function object is now in a box, the size_of Value has gone from 280 to 8 |
|
The error in the benchmarks is because I actually fixed the JavaScript code in the benchmark and now we reach https://github.com/jasonwilliams/boa/blob/master/boa/src/exec/mod.rs#L574 since for loops are not implemented. Maybe we should remove that benchmark until we actually have the for loop execution implemented. |
Razican
reviewed
May 2, 2020
Benchmark for 794e565Click to view benchmark
|
Razican
approved these changes
May 2, 2020
|
If it's possible could you wait. I would like to see what I can optimize, before merging :) |
Co-authored-by: Iban Eguia <[email protected]>
Benchmark for 7d4b663Click to view benchmark
|
|
HalidOdat
approved these changes
May 3, 2020
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.
Fixes #141