Bug 175818

Summary: Speedometer: Make React/Redux item order consistent
Product: WebKit Reporter: Mathias Bynens <mathias>
Component: New BugsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: addyo, buildbot, cdumez, joepeck, koivisto, mathias, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 172339    
Attachments:
Description Flags
Patch
none
Fixes the bug none

Description Mathias Bynens 2017-08-22 02:20:26 PDT
Speedometer: Make React/Redux item order consistent
Comment 1 Mathias Bynens 2017-08-22 02:22:07 PDT
Created attachment 318749 [details]
Patch
Comment 2 Ryosuke Niwa 2017-08-22 13:51:06 PDT
Comment on attachment 318749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318749&action=review

Don't you need to rebuild? We're loading bundle files in the actual test?

> PerformanceTests/Speedometer/resources/todomvc/architecture-examples/react-redux/src/reducers/todos.js:14
> +        ...state,
>          {
>            id: state.reduce((maxId, todo) => Math.max(todo.id, maxId), -1) + 1,
>            completed: false,
>            text: action.text
> -        },
> -        ...state
> +        }

Hm... I swear I tried this and didn't work :(
Comment 3 Ryosuke Niwa 2017-08-22 16:57:35 PDT
Comment on attachment 318749 [details]
Patch

As I suspected, the problem is not fixed if I just apply the patch.
Comment 4 Ryosuke Niwa 2017-08-22 23:20:33 PDT
Created attachment 318854 [details]
Fixes the bug
Comment 5 Mathias Bynens 2017-08-22 23:41:30 PDT
Comment on attachment 318854 [details]
Fixes the bug

LGTM.
Comment 6 Mathias Bynens 2017-08-22 23:43:11 PDT
(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 318749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318749&action=review
> 
> Don't you need to rebuild? We're loading bundle files in the actual test?
> 
> > PerformanceTests/Speedometer/resources/todomvc/architecture-examples/react-redux/src/reducers/todos.js:14
> > +        ...state,
> >          {
> >            id: state.reduce((maxId, todo) => Math.max(todo.id, maxId), -1) + 1,
> >            completed: false,
> >            text: action.text
> > -        },
> > -        ...state
> > +        }
> 
> Hm... I swear I tried this and didn't work :(

Yeah, I forgot to include the re-built files — sorry about that.

The code change is correct though. I’ve upstreamed it here along with tests: https://github.com/reactjs/redux/pull/2575
Comment 7 Joseph Pecoraro 2017-08-23 12:21:34 PDT
Comment on attachment 318854 [details]
Fixes the bug

rs=me
Comment 8 Ryosuke Niwa 2017-08-23 13:08:37 PDT
There doesn't appears to be a measurable difference in score for this subtest (each number is the average of 10 iterations).

== Before ==
Safari
74.35
75.24
74.47
74.80

Chrome
88.20
88.38
88.29
89.68

Firefox
44.24
43.50
44.74
44.12

== After ==
Safari
75.19
74.88
74.22
74.57

Chrome
88.74
89.00
89.47
89.60

Firefox
41.47
44.09
43.42
44.12
Comment 9 Ryosuke Niwa 2017-08-23 14:39:58 PDT
Comment on attachment 318854 [details]
Fixes the bug

Clearing flags on attachment: 318854

Committed r221105: <http://trac.webkit.org/changeset/221105>
Comment 10 Ryosuke Niwa 2017-08-23 14:40:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-08-23 14:40:53 PDT
<rdar://problem/34044454>