WebKit / Speedometer

An open source repository for the Speedometer benchmark
Other
592 stars 70 forks source link

Always instantiate Float32Array in the cost array of findOptimalSegmentationInternal. #342

Closed rniwa closed 8 months ago

rniwa commented 8 months ago

Chrome 120.0.6099.199

Before 29.3 ± 1.0 26.4 ± 1.4 27.4 ± 1.4

After 27.5 ± 2.4 28.6 ± 1.6 27.9 ± 1.0

Firefox 121.0.1

Before 21.9 ± 1.7 22.0 ± 1.6 21.7 ± 1.7

After 22.4 ± 1.9 22.1 ± 1.9 21.8 ± 1.5

Safari 17.2

Before 23.3 ± 0.90 23.8 ± 1.3 23.5 ± 1.4

After 24.8 ± 0.70 24.1 ± 1.3 24.3 ± 0.76

julienw commented 8 months ago

Do you have more context about this change?

My understanding is that it's a fairly trivial change, please tell me if there's anything more than consistency.

rniwa commented 8 months ago

Do you have more context about this change?

My understanding is that it's a fairly trivial change, please tell me if there's anything more than consistency.

The context is that cost array is always intended to be an array of Float32Array but we were forgetting to instantiate Float32Array for the very first item in the array. This PR makes it more consistent across the array.

julienw commented 8 months ago

Ok so it's clearly trivial

jrmuizel commented 8 months ago

@rniwa can you fix the same problem in JetStream too? https://github.com/WebKit/WebKit/blob/c6d8b99f59c25be18018634dc25fff0d66022287/PerformanceTests/JetStream2/worker/segmentation.js#L395

rniwa commented 8 months ago

@rniwa can you fix the same problem in JetStream too? https://github.com/WebKit/WebKit/blob/c6d8b99f59c25be18018634dc25fff0d66022287/PerformanceTests/JetStream2/worker/segmentation.js#L395

Hm... I guess that would probably mean releasing JetStream 2.2 since 2.1 is already out? I'd create a PR but I'd leave it to folks maintaining JetStream to weigh in.

rniwa commented 8 months ago

Made a PR for JetStream 2 at https://github.com/WebKit/WebKit/pull/22688