538326 - Large regression in test case from bug 534590

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Before this patch:
host-5-37:src dvander$ ./Opt32/js -j b.js 
124999999567108900
1893 ms

After:
host-5-37:src dvander$ ./Opt32/js -j b.js 
124999999567108900
6204 ms

Test case is from dmandelin who got the problem on his machine (OS X 10.5). I can reproduce it on 10.6 & Linux. The interpreter is slower as well, though not by as much.

The patch in bug 534590 seems harmless so at first I thought hg bisect was lying, but this is very reproducible for me. From looking at Shark it seems like all the time is purely on trace, and that there might be more L2 cache misses.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: test case

Comment 2

[Andreas Gal :gal]

I would check for alignment problems.

Comment 3

[Igor Bukanov]

(In reply to comment #0)
> there might be more L2 cache misses.

The patch contains:

===================================================================
--- tm.orig/js/src/jscntxt.h	2009-12-23 13:58:34.311884714 +0300
+++ tm/js/src/jscntxt.h	2009-12-23 13:59:30.970882813 +0300
@@ -553,17 +553,17 @@ struct JSRuntime {
-    JSDHashTable        *gcLocksHash;
+    JSDHashTable        gcLocksHash;


That is, it replaces a pointer field in JSRuntime by JSDHashTable, a struct with sizeof == 32 bytes on Linux. That may be enough to make access to the fields after it slower. 

What would happen if gcLocksHash is moved at the end of JSRuntime?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Thanks Igor, that was the problem. Brendan suggested bisecting to which struct member was misaligned, and it ended up being JSThreadData. From there Luke remembered that our native stack comes from JSThreadData.traceMonitor.storage. For some reason GCC isn't 16-byte aligning the double.

So this patch just mallocs the storage area instead.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Created an attachment (id=420640) [details]
> fix
> 
> Thanks Igor, that was the problem. Brendan suggested bisecting to which struct
> member was misaligned, and it ended up being JSThreadData. From there Luke
> remembered that our native stack comes from JSThreadData.traceMonitor.storage.
> For some reason GCC isn't 16-byte aligning the double.

Or even 8-byte aligning it. /me shakes fist at GCC

Shell-only regression, but the compiler's failure to align for performance fills me with dread.

/be

Comment 6

[Brendan Eich [:brendan]]

Why malloc, though? It adds indirection. Shouldn't we just static-assert that the double array is double-aligned and hack or pragma until it is?

/be

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

I don't think the indirection matters too much here, and it's easier than mucking with compiler/platform-specific gunk.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/tracemonkey/rev/404c6c7b32c3

Comment 9

[Guilherme Lima]

Sorry to bother you but did this patch land on mozilla-central? Marked as fixed-in-tracemonkey in January, but not as resolved fixed..

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/404c6c7b32c3