-
-
Notifications
You must be signed in to change notification settings - Fork 477
Object specialization #419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
928ee40 to
62bdd68
Compare
Benchmark for 3d93be3Click to view benchmark
|
62bdd68 to
5c4dc6f
Compare
Benchmark for 36cc48eClick to view benchmark
|
|
@Razican and @jasonwilliams I'm having a bit of trouble with the garbage collector. Any help would be useful |
Benchmark for b52be12Click to view benchmark
|
I'm not very used to deal with the Gc, but this seems to be that two calls to |
Benchmark for b1ff14cClick to view benchmark
|
|
I did some benchmarks on Object creation and access I think the other execution benchmarks are wrong, we use You can check the benchmark here What do you think @Razican and @jasonwilliams ? |
@jasonwilliams noticed that most of the execution time is spent on Realm creation, so I think we should isolate the actual execution, yes, in order to see if we improve or worsen there. |
|
I think we need to make some benchmark issues and get them in before this so we can have some before/after |
Benchmark for d0b4745Click to view benchmark
|
Benchmark for 345bcc1Click to view benchmark
|
Benchmark for 38d0eb1Click to view benchmark
|
896ba46 to
d3d7fb8
Compare
Benchmark for 494d109Click to view benchmark
|
Razican
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, an extra bunch of comments. I think one of the best things we could do is using an Object directly instead of a Value as the global object when initializing the Realm.
|
|
||
| prototype | ||
| .as_object_mut() | ||
| .unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has not been changed, right?
| parent.set_field(Value::from(name_copy), new_func_obj); | ||
| parent | ||
| .as_object_mut() | ||
| .unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not changed, right?
boa/src/exec/mod.rs
Outdated
| self.throw_type_error("cannot convert null or undefined to Object")?; | ||
| unreachable!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this in this PR or in a different one?
d19ed61 to
598adfe
Compare
Benchmark for 31c4c6cClick to view benchmark
|
|
We still don't have benchmarks for the specialized objects 😅 , Like Edit: Made some benchmarks for some specialized objects #494 |
Benchmark for 79188c1Click to view benchmark
|
|
We should do a before and after with the profiler, it would be nice to see |
|
Here is for And for |
Benchmark for 3b70830Click to view benchmark
|
Benchmark for c9dcfd8Click to view benchmark
|
Razican
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to go from my side. We might want to wait for the extra benchmarks though, to have some extra info.
- Added helper functions for Object and ValueData - Added `Hash` trait to `Value` - Abstracted Object field access - Deprecated `ObjectInternalMethods` - Optimized Realm creation - Fixed `Symbol` primitive
hash The problem with using the pointer as the hash, it can become a security vulnerability.
This type change removes 8-bytes from Symbol type size.
- Added `#[inline]` to some small functions
ecd65b2 to
25572f5
Compare
Benchmark for 79c718aClick to view benchmark
|
|
Lets merge this :) |


It changes the following:
Hashtrait forValuePartialEqtrait based on theSameValueZeroalgorithm forValue(required byHashtrait)Eqtrait forValue(required byHashtrait)same_value_non_numberObjectInternalMethodsconstruct_type_errorandconstruct_range_errorNumberobjects constructor and methods more specification compliant.initfunctions to return(&str, Value)The problem with the current implementation is that we store the data in an internal slot. The internal slots are stored as a
HashMap, which means slow reads and writes. Additionally they are stored as aValuewhich means we need additional checks to unwrap the Value into a primitive (examplef64,String) to access the data. Furthermore we are restricted as to what type of data we can hold, for example if we wanted to implementSetobject with native typeHashSet, we can not do so in the internal slots as they are specified in the standard (it's only an implementation detail).This PR aims to allow use of non-
Valuetypes in objects as well as removing the need for internals slots for data storage (NumberData,StringData, etc), removing redundant checks for data access. It should increase performance greatly for object access and a bit for object creation. (I'm not sure the benchmarks will show it though, since we do not have benchmarks for objects).Specialized objects: