Reduce memory allocations #452
Conversation
Jint/Engine.cs
Outdated
| public void DeclarationBindingInstantiation(DeclarationBindingType declarationBindingType, IEnumerable<FunctionDeclaration> functionDeclarations, IEnumerable<VariableDeclaration> variableDeclarations, FunctionInstance functionInstance, JsValue[] arguments) | ||
| public void DeclarationBindingInstantiation( | ||
| DeclarationBindingType declarationBindingType, | ||
| IList<FunctionDeclaration> functionDeclarations, |
There was a problem hiding this comment.
IList gives us indexing but not allocation free foreacher, so using indexing
There was a problem hiding this comment.
Is there a reason to not use List<T> here?
There was a problem hiding this comment.
In particular, I'm thinking about bound check elimintations.
There was a problem hiding this comment.
The problem here is (like other places too) that we are dealing with public API. Changing to concrete implementations might not be the prettiest thing, even though best performing. I think @sebastienros can be the judge of this.
Jint/Engine.cs
Outdated
| var varAlreadyDeclared = env.HasBinding(dn); | ||
| if (!varAlreadyDeclared) | ||
| var variableDeclaration = variableDeclarations[i]; | ||
| var declarations = (List<VariableDeclarator>) variableDeclaration.Declarations; |
There was a problem hiding this comment.
this is a repeating pattern, we know Esprima and that it produces List, we can cast to it can get non-virtual dispatch + allocation free foreach
There was a problem hiding this comment.
Might be worth just changing it there, no? That would give also skip the type checks here.
There was a problem hiding this comment.
I have a pending pull request sebastienros/esprima-dotnet#13 that optimizes things, but does not change the API. Same thing here about setting the public API to concrete implementations. I guess streaming API like IEnumerable will not be supported in the near future and everything is materialized as is.
Forced cast is pretty much safe though as unit tests cover well all these and upgrading to different kind of Esprima would show up.
There was a problem hiding this comment.
I've created separate pull request to change Ast to have concerete types: sebastienros/esprima-dotnet#14
There was a problem hiding this comment.
Concrete type PR went in, but requires NuGet release to be used here.
There was a problem hiding this comment.
Pushed on Nuget, should appear soon.
|
I've upgraded to latest Esprima and cleaned the casts, removed Linq from DefaultTypeConverter too. I unfortunately missed the HoistingScope members but they have an API cleanup PR pending that fixes them too. Can we merge at this point? |
lahma
force-pushed
the
perf/remove-linq-from-hot-paths
branch
3 times, most recently
from
f719d32
to
9da94db
Compare
lahma
force-pushed
the
perf/remove-linq-from-hot-paths
branch
from
9da94db
to
a247d12
Compare
This time around again trying to reduce memory allocations to give virtual machine some breathing room. Most of the changes should have positive changes all around.
Next I'm going to tackle generally ObjectInstance and it's memory usage, trying to optimize MruPropertyCache2 etc.
UncacheableExpressionsBenchmark
Before (dev branch)
After (this pull request)
ArrayStressBenchmark
Before
After