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

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

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Jan 10, 2024

No description provided.

@@ -674,7 +674,7 @@ addMetric(metric){this._metrics.push(metric);}
isHidden(){return this._hidden;}}
if(typeof module!='undefined')
module.exports.Test=Test;class Metric extends LabeledObject{constructor(id,object)
{super(id,object);this._aggregatorName=object.aggregator;object.test.addMetric(this);this._test=object.test;this._platforms=[];const suffix=this.name().match('([A-z][a-z]+|FrameRate)$')[0];this._unit={'FrameRate':'fps','Runs':'/s','Time':'ms','Duration':'ms','Malloc':'B','Heap':'B','Allocations':'B','Size':'B','Score':'pt','Power':'W',}[suffix];}
{super(id,object);this._aggregatorName=object.aggregator;object.test.addMetric(this);this._test=object.test;this._platforms=[];const suffix=this.name().match('([A-Z][a-z]+|FrameRate)$')[0];this._unit={'FrameRate':'fps','Runs':'/s','Time':'ms','Duration':'ms','Malloc':'B','Heap':'B','Allocations':'B','Size':'B','Score':'pt','Power':'W',}[suffix];}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, this is a weird typo in the built script. I wonder how that happened?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the meeting, this was likely from not regenerating the output after fixing it in #282 .

…ntationInternal.

findOptimalSegmentationInternal's cost array was intended to always contain Float32Array
instead of mixture of normal Array and Float32Array. This PR fixes it for the very first
element in the cost array, which was erroneously creating a normal Array

This PR also fixes a typo in the regular expression of the bundled script where it had
[A-z] instead of [A-Z].
@rniwa rniwa force-pushed the fix-segmentation-float32array branch from fb6e6b2 to 9c4c2be Compare January 10, 2024 05:40
@rniwa
Copy link
Member Author

rniwa commented Jan 10, 2024

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

@bgrins bgrins requested a review from julienw January 10, 2024 18:40
@rniwa rniwa added the non-trivial change A change that affects benchmark results label Jan 10, 2024
@julienw
Copy link
Contributor

julienw commented Jan 11, 2024

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
Copy link
Member Author

rniwa commented Jan 11, 2024

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
Copy link
Contributor

julienw commented Jan 11, 2024

Ok so it's clearly trivial

@jrmuizel
Copy link

@rniwa rniwa merged commit 2aa819f into main Jan 11, 2024
4 checks passed
@rniwa
Copy link
Member Author

rniwa commented Jan 12, 2024

@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 rniwa deleted the fix-segmentation-float32array branch January 12, 2024 00:05
@rniwa
Copy link
Member Author

rniwa commented Jan 12, 2024

Made a PR for JetStream 2 at WebKit/WebKit#22688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-trivial change A change that affects benchmark results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants