-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Object specialization #419
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
|
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
|
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:
Hash
trait forValue
PartialEq
trait based on theSameValueZero
algorithm forValue
(required byHash
trait)Eq
trait forValue
(required byHash
trait)same_value_non_number
ObjectInternalMethods
construct_type_error
andconstruct_range_error
Number
objects constructor and methods more specification compliant.init
functions 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 aValue
which 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 implementSet
object 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-
Value
types 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: