Skip to content
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

Introduce IPropertyDescriptor and specialized descriptors #461

Merged

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Jan 8, 2018

This PR changes PropertyDescriptor usage to happen through IPropertyDescriptor interface. This allows specialized descritors that don't hold other instance fields other than the actual JsValue. This benefits especially arrays where descriptor stays quite constant. Array heave operators seem to get around 10% allocation reduction.

ObjectInstance converts to more memory hungry full PropertyDescriptor when changes are requested. I've kept new specialized versions as internal. This change affects memory usage and should not have much positive or negative impact on speed. Virtual dispatch is already happening with PropertyDescriptor.Value as it's virtual.

Before

Method N Mean Error StdDev Gen 0 Allocated
Slice 100 877.4 us 7.603 us 7.112 us 239.2578 982.81 KB
Concat 100 991.0 us 1.550 us 1.210 us 257.8125 1062.5 KB
Unshift 100 36,026.6 us 182.054 us 152.023 us 7000.0000 28785.16 KB
Push 100 23,722.6 us 439.804 us 411.393 us 1625.0000 6762.5 KB
Index 100 25,124.1 us 59.588 us 49.759 us 1531.2500 6333.59 KB
Map 100 4,644.9 us 15.578 us 13.810 us 1804.6875 7420.31 KB
Apply 100 1,040.4 us 5.970 us 5.584 us 267.5781 1098.44 KB
JsonStringifyParse 100 6,641.6 us 26.941 us 25.200 us 1476.5625 6068.75 KB
Method N Mean Error StdDev Gen 0 Gen 1 Allocated
UncacheableExpressionsBenchmark 500 572.1 ms 82.07 ms 4.637 ms 114750.0000 33562.5000 494.73 MB

After

Method N Mean Error StdDev Gen 0 Allocated
Slice 100 839.7 us 6.128 us 5.432 us 154.2969 635.94 KB
Concat 100 947.8 us 11.131 us 10.412 us 171.8750 707.81 KB
Unshift 100 37,094.3 us 278.159 us 260.190 us 3750.0000 15492.19 KB
Push 100 25,588.8 us 30.847 us 27.345 us 1468.7500 6125.78 KB
Index 100 26,047.0 us 102.136 us 90.541 us 1375.0000 5696.88 KB
Map 100 4,650.9 us 9.938 us 9.296 us 1718.7500 7071.88 KB
Apply 100 1,038.3 us 3.905 us 3.653 us 187.5000 774.22 KB
JsonStringifyParse 100 6,671.2 us 6.482 us 6.063 us 1390.6250 5710.16 KB
Method N Mean Error StdDev Gen 0 Gen 1 Allocated
UncacheableExpressionsBenchmark 500 592.7 ms 22.18 ms 1.253 ms 100104.1667 29541.6667 446.99 MB

@lahma lahma force-pushed the perf/property-descriptor-interface branch from 4d04a52 to 02b0d7e Compare January 8, 2018 17:53
@sebastienros
Copy link
Owner

Looks good.

@lahma
Copy link
Collaborator Author

lahma commented Jan 8, 2018

Thanks. End user API changes with IPropertyDescriptor being returned/taken as parameter (PropertyDescriptor works as before for defining things). Also the methods IsDataDescriptor() etc moved to be extension methods to help interface-only implementations.

Feel free to merge if/when you find suitable, all tweaks naturally welcome.

@sebastienros
Copy link
Owner

I was just wondering if it would be better to introduce [Flags]. I know I have tried by the past, but I think it was more pain than this. Unless there are helper methods.

@lahma
Copy link
Collaborator Author

lahma commented Jan 8, 2018

Yes, that's an alternative. Without interface it would marginally faster. Of course the flags need to be stored as field but getters could do bit checks. The extra Get and Set would be hard to model without properties though.

But I'm in no hurry with this one. The last merge brought the speed improvements already, this is tweaking and balancing now.

@sebastienros sebastienros merged commit f5f4329 into sebastienros:dev Jan 8, 2018
@lahma lahma deleted the perf/property-descriptor-interface branch February 11, 2018 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants