-
-
Notifications
You must be signed in to change notification settings - Fork 557
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
StringDictionarySlim instead of Dictionary/StructDictionary #554
Conversation
…tDictionary * remove intrinsic properties
131695c
to
4531ced
Compare
@@ -18,8 +19,7 @@ namespace Jint.Native.Object | |||
{ | |||
public class ObjectInstance : JsValue, IEquatable<ObjectInstance> | |||
{ | |||
protected Dictionary<string, PropertyDescriptor> _intrinsicProperties; | |||
protected internal Dictionary<string, PropertyDescriptor> _properties; | |||
internal StringDictionarySlim<PropertyDescriptor> _properties; |
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.
Why a single dictionary now? Didn't I do two dictionaries on purpose?
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.
Well I couldn't find the reason to have separate _intrinsicProperties, tried to read the ECMA spec and it doesn't have such distinction. Symbolic properties work the same way as normal properties etc.
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.
I think it might be for iterating. Pretty sure it was necessary.
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.
Let's see how it goes without it then, next time I think something matters I'll write a unit test ;)
_properties?.Clear(); | ||
_intrinsicProperties?.Clear(); | ||
|
||
_properties = null; |
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.
Wasn't the goal here to reuse the same instance?
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.
The new data type doesn't support Clear, didn't make much of a difference as you can see from perf report. ArgumentsInstances are the big polluters as they were created for function calls, might not have properties materialized that often.
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.
You can only say if you run a diff with and without ;)
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.
Shouldn't be hard to implement, will add.
* HashHelpers to StringDictionarySlim
@@ -298,14 +301,14 @@ public JsValue Freeze(JsValue thisObject, JsValue[] arguments) | |||
{ | |||
if (desc.Writable) | |||
{ | |||
var mutable = desc as PropertyDescriptor ?? new PropertyDescriptor(desc); |
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.
Why aren't we cloning it anymore? Are these immutable?
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.
R# showed that this will never be evaluated, every property descriptor is a PropertyDescriptor..
* support Clear
I have replaced the StructDictionary with slightly modified DictionarySlim. This version uses string keys directly (hence the name StringDictionarySlim) and doesn't check for collisions as we don't need to detect multi-threaded access.
I also removed separate dictionary _intrinsicProperties as so far I've seen need, we can probably add back later if needed.
Performance results generally seem to be within the waving my laptop does, but the greatest benefit can be seen in UncacheableExpressionsBenchmark where more realistic object mapping happens against the new dictionary type.
Esprima.Benchmark.DromaeoBenchmark
Jint.Benchmark.ArrayBenchmark
Jint.Benchmark.UncacheableExpressionsBenchmark