add more benchmarks #447
add more benchmarks #447
Conversation
|
There is an updated version of the article here https://rushfrisby.com/net-javascript-engine-performance-results-updated-2016/ And it looks like the author is using Jint even though it's the slowest on these scripts. It's not unexpected as these are compute intensive, and compiled scripts will shine, so V8 will always win there. But that a good set of benchmarks to find the big bottlenecks. Obviously Array is one of the main issues, which is why you will find a branch here were I tried to optimized things, without much success. If you want to improve it, the main idea would be to have different implementations of the prototype methods (push, iterate, sort, ...) based on the type of array, like sparse, range, ... This is what v8 does too. The idea is that the specification states an array can be sparse and it uses string indices. But it makes the algorithm and implementations slow. So to optimize it we can store some flags and detect best cases, then not use the exact specification but optimized algorithms. I think V8 supports three different types. |
|
A few comments:
|
|
Thank you for reviewing. In this pull request I'm just trying put some baseline numbers available. I did see the updated benchmark which brings the sunspider etc tests in, but thought this would be easier as first step. As the sunspider tests seem to be part of tests already they should not be hard to reference. What I understood I should split the performance pull requests (esprima, jint array performance) to smaller parts which I fully understand, but is this PR good as-is or should I split it to smaller parts? I probably need to check the reused engine. It might not affect as loop is now N=1 and engine should be probably a static variable in this case. |
|
I've updated the the benchmark and top comment with run information done with 5 repetitions per new engine instance / shared engine. The array handling at the moment is a bit slow and thus takes some time to get results. With friendlier benchmark (not targeting array handling) we can use larger N and see a bigger difference by using the same instance. |
|
Just some words about these kind of benchmarks. We did a whole bunch of work to see what it would look like with Jurassic on our end. The kind of benchmark you are seeing here is misleading, because you are doing a LOT of work inside the js engine. If you are using it to mostly do things outside, such as directing the operation of other code, than the cost of going in and out of the engine can kill your perf. See: https://ayende.com/blog/179553/with-performance-test-benchmark-and-be-ready-to-back-out |
|
I am ok with tracking perf comparisons with Jurassic and NilJS, at least to know the differences, as long as we can see different scripts, like some sunspiders but also small ones like the one which was used initially. Can also show the scenarios that @ayende is referring to. This should be one benchmark table. Then we can have other specific benchmarks that don't show Jurassic or NiJS which would only be used to track improvements and regressions. Arrays being one of them. In this case we really don't care about the But before doing any change I want to hear what everyone has to say. You two have the most weight on the matter as you are directly impacted. |
|
I had the different engines there as the original benchmark project had the Jurassic one and the per comparison seemed like a way to see "how far are we". I do understand the fundamental differences between engines. For me this more of fun optimization exercise that hopefully could benefit both Jint and RavenDB. My journey began with some JSON projection trouble that seemed to use a lot of memory. I'm open to drop comparison between engines or change to method level benchmarks. I just need some torture baseline like the array case which is worst case scenario, but showed nicely an area to improve on and a number for PR to reflect against. |
|
I've now put the other engine benchmark behind conditional compilation and they won't be present by default. I've also added the sun spider benchmarks as separate benchmark that gives results for each file, see results below. Is there something more to do/change before this could be merged?
|
lahma
force-pushed
the
more-benchmarks
branch
from
323ea8c
to
b7cdb34
Compare
|
|
Currently reviewing it. I might refactor it to my taste if you don't mind, I don't want to ask you to do more changes and get you frustrated, you already did a lot ;) Only details, don't worry. |
| } | ||
|
|
||
| [Params(500)] | ||
| public int N { get; set; } |
There was a problem hiding this comment.
I don't understand this. Why is it necessary when Benchmarks.NET can already do that by itself?
There was a problem hiding this comment.
Using Params adds it to benchmark report and clarifies the benchmark case's scenario. Benchmark.NET runs the target method X times which it finds best to get good confidence intervals etc. So here I want to ad to report "The target Jint function was called 500 times inside the test case". We could also add more numbers that would show how it behaves depending on iteration count (if it would be costly to call once due to caches but really fast after that).
|
@sebastienros please do any necessary changes, I'm more than OK with that, thanks. |
|
|
||
| private static void InitializeEngine(Options options) | ||
| { | ||
| options |
There was a problem hiding this comment.
Ignore, didn't see it was specific to this benchmark
There was a problem hiding this comment.
Yes and ones that RavenDB uses, MaxStatements especially trips here to error when the smaller default that RavenDB uses.. something like 50 in there and far from enough.
| /// Test case for situation where object is projected via filter and map, Jint deems code as uncacheable. | ||
| /// </summary> | ||
| [Config(typeof(Config))] | ||
| public class UncacheableExpressionsBenchmark |
There was a problem hiding this comment.
Looks like a special case benchmark. If there is a bug (like something should be cached and is not) we should have a unit test. If this is just to make a specific case fast enough, then it shouldn't be in the repos, or at least on a custom branch where the work for improvement will happen, then be removed once it's done.
There was a problem hiding this comment.
In a way a special case, but this should reflect function invocation with arrays in quite generic way. Arrays struggle with these kinds of data where index access/traversal goes via dictionary instead of pure array indexing and produces more arrays.
There isn't a bug per se, but a performance problem when you try to filter/project large nested arrays. But feel free to remove if you want.
There was a problem hiding this comment.
map and filter are very common things to do on arrays.
And they can usually be lifted quite easily, that is not just a generic scenario, that is something that I think should be optimized specifically
There was a problem hiding this comment.
Then these benchmarks should be in the same table as the sunspider ones, unless there are already included there, or within one dedicated to Array operations.
There was a problem hiding this comment.
In an Array benchmark class that would look like this http://jsben.ch/k3QoV. It doesn't have to be in this PR. We can do it later or as part of a perf PR that would focus on arrays.
|
I've renamed the old ArrayBenchmark to ArrayStressBenchmark and created new ArrayBenchmark for the latest link that you gave. I included the the operations that are supported. There might be problems combining benchmark suites and I personally don't follow the same mindset that you would with unit/integration tests. Ideally benchmark case would have two methods: old algorithm and new algorithm which would create us the tables etc nicely. This however, is not easy to achieve when you are trying to make larger changes/refactorings - then I usually run the suite in "master" (or base branch containing only new benchmark against master), copy result directory, checkout to my new branch and run same test and then create the comparison results by showing the before and after reports. It's also problematic when you have too many tests in benchmark, it becomes slow to run when you just want to optimize some very specific thing (maybe one test method shows that one). Unlike tests and the actual shipped code, benchmarks are more in supporting role and might not be reused that much depending on the case. I hope we find some middle ground soon, I'd need the two optimizations to advance other analysis, it's a bit painful to cherry-pick between branches and trying to mentally keep up what's unfixed problem and what is fixed somewhere else. |
|
Thank you for handling the PR fast , my eagerness to improve may show up as impatience but I hope I can help the best I can. |
|
I've created a new issue #451 to track overall progress, I'll update results there between pull requests. |
This already shows that some of array usage problems might be performance related, I'm going to investigate if there are any easy wins.