Bug 143031

Summary: New map and set modification tests in r181922 fails
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Critical CC: bfulgham, buildbot, cgarcia, fpizlo, ggaren, mark.lam, msaboff, oliver, ossy, rniwa, ysuzuki
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 142696    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Patch
none
Patch none

Description Mark Lam 2015-03-24 19:12:32 PDT
** The following JSC stress test failures have been introduced:
	stress/modify-map-during-iteration.js.always-trigger-copy-phase
	stress/modify-map-during-iteration.js.default
	stress/modify-map-during-iteration.js.dfg-eager
	stress/modify-map-during-iteration.js.dfg-eager-no-cjit-validate
	stress/modify-map-during-iteration.js.no-cjit-validate-phases
	stress/modify-map-during-iteration.js.no-llint
	stress/modify-set-during-iteration.js.always-trigger-copy-phase
	stress/modify-set-during-iteration.js.default
	stress/modify-set-during-iteration.js.dfg-eager
	stress/modify-set-during-iteration.js.dfg-eager-no-cjit-validate
	stress/modify-set-during-iteration.js.no-cjit-validate-phases
	stress/modify-set-during-iteration.js.no-llint
Comment 2 Yusuke Suzuki 2015-03-24 19:15:36 PDT
I'm now preparing a patch to fix it simply.
https://bugs.webkit.org/show_bug.cgi?id=142696
Comment 3 Yusuke Suzuki 2015-03-24 19:31:22 PDT
Created attachment 249377 [details]
Patch
Comment 4 Yusuke Suzuki 2015-03-24 19:34:34 PDT
Comment on attachment 249377 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:53
> +        when adjusting m_index. This is a little bit ugly, but it eliminates heap stored MapDataPatch.

Alternative fix is iterating twice.
1st:
Calculating newEnd and packing to destination by destination[newEnd] = entry;
2nd:
Iterating in reverse order and decrementing m_index.
Comment 5 Build Bot 2015-03-24 20:29:56 PDT
Comment on attachment 249377 [details]
Patch

Attachment 249377 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6471842135015424

New failing tests:
fast/regions/auto-size/autoheight-two-pass-layout-complex-002.html
Comment 6 Build Bot 2015-03-24 20:29:59 PDT
Created attachment 249381 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Alexey Proskuryakov 2015-03-24 21:02:02 PDT
I don't think that it's Windows only, they fail on Mac too. 

The EWS failure is unrelated.
Comment 8 Yusuke Suzuki 2015-03-25 00:54:02 PDT
Created attachment 249389 [details]
Patch

Just updated the title of ChangeLog since this is not limited in Windows environment
Comment 9 Geoffrey Garen 2015-03-25 10:25:31 PDT
Oops! This will teach me to land a patch and then go home :(.
Comment 10 Geoffrey Garen 2015-03-25 10:55:35 PDT
Comment on attachment 249389 [details]
Patch

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

> Source/JavaScriptCore/runtime/MapData.h:73
> +            if (m_oldIndexUsedDuringPacking <= index)

I think we can accomplish this without an extra pass to save our old index. I'll post another tweak to do this.
Comment 11 Geoffrey Garen 2015-03-25 11:35:56 PDT
Created attachment 249417 [details]
Patch
Comment 12 Geoffrey Garen 2015-03-25 11:37:58 PDT
Committed r181968: <http://trac.webkit.org/changeset/181968>
Comment 13 Yusuke Suzuki 2015-03-26 01:28:10 PDT
Comment on attachment 249389 [details]
Patch

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

>> Source/JavaScriptCore/runtime/MapData.h:73
>> +            if (m_oldIndexUsedDuringPacking <= index)
> 
> I think we can accomplish this without an extra pass to save our old index. I'll post another tweak to do this.

Great! Your tweak is very reasonable. Thank you!