New issue
Advanced search Search tips

Issue 855582 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 25
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Changing Array.p.sort to a stable sort causes layout test to fail

Project Member Reported by szuend@google.com, Jun 22 2018

Issue description

We are currently experimenting with a stable sorting algorithm for Array.p.sort (TimSort). We planned to land it for a few hours to gather performance/stability feedback. Unfortunately it brook a devtools layout test (last-execution-context.js).

Link to build: https://ci.chromium.org/buildbot/client.v8.fyi/V8-Blink%20Linux%2064/24236
Link to failing test summary: https://test-results.appspot.com/data/layout_results/V8-Blink_Linux_64/24236/layout-test-results/results.html

My question now is whether this change really breaks something, or is it enough to re-baseline the test?
 
Cc: -kozyatinskiy@chromium.org
Labels: -Pri-3 Pri-2
Owner: kozyatinskiy@chromium.org
Status: Assigned (was: Untriaged)
Alexey, could you please have a brief look? This is currently blocking our progress on the Array.p.sort rewrite. Thanks :)
Cc: szuend@google.com
From offline discussion: we'll skip this layout test for our next experiment. It's still TBD if the test relies on unspecc'd sort behavior or if the problem is somewhere else.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25e38b9017e062b6dbeac65c0cc33b9b5e0a0fe3

commit 25e38b9017e062b6dbeac65c0cc33b9b5e0a0fe3
Author: Simon Zünd <szuend@google.com>
Date: Wed Aug 08 16:00:32 2018

Mark layout test "last-exeuction-context.js" as failure/pass

The CL https://crrev.com/c/1151199 will break this layout test.
In preparation for landing the CL we mark it as Failure/Pass and
rebaseline after the roll.

Bug:  chromium:855582 
Change-Id: I57ce0dbdccf403a14a0d6021cdac9111a8a60ae6
Reviewed-on: https://chromium-review.googlesource.com/1166913
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Commit-Queue: Simon Zünd <szuend@google.com>
Cr-Commit-Position: refs/heads/master@{#581581}
[modify] https://crrev.com/25e38b9017e062b6dbeac65c0cc33b9b5e0a0fe3/third_party/WebKit/LayoutTests/TestExpectations

Owner: kozy@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95cbfdf90d8011bf02576a4a1895d893c3def039

commit 95cbfdf90d8011bf02576a4a1895d893c3def039
Author: Alexey Kozyatinskiy <kozyatinskiy@chromium.org>
Date: Tue Sep 25 00:42:58 2018

DevTools: reenable last-execution-context.js test

This test should not rely on stability of V8 sort algorithm.

TBR=dgozman@chromium.org

Bug:  chromium:855582 
Change-Id: Ia1678da6d4ba8a04d5b7135f34e1748632414720
Reviewed-on: https://chromium-review.googlesource.com/1242042
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593780}
[modify] https://crrev.com/95cbfdf90d8011bf02576a4a1895d893c3def039/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/95cbfdf90d8011bf02576a4a1895d893c3def039/third_party/WebKit/LayoutTests/http/tests/devtools/sources/debugger-ui/last-execution-context.js

Status: Fixed (was: Assigned)

Sign in to add a comment