531225 - Workers: Share strings across thread boundary

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

We're copying now, and it's unfortunate. We should share while we can.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

jst, review ping!

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 414680 [details] [diff] [review]
Patch, v1

Duh, sorry for the way too long delay :(

r=jst

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/bdb83d642185

Comment 4

[Christopher Blizzard (:blizzard)]

How safe would this be for 3.6.x?  Could be a big perf improvement for a lot of people writing JS stuff out there.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #4)
> How safe would this be for 3.6.x?  Could be a big perf improvement for a lot of
> people writing JS stuff out there.

I can't think of any reason this would be risky, so after it bakes for a bit I'd be all for backporting. The code hasn't really changed much since 3.5.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 414680 [details] [diff] [review]
Patch, v1

I'd like to take this on branch since it avoids copies of data passed by workers. Pretty much every single test for workers exercises this code so we'd have known pretty fast if this change had broken anything.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 414680 [details] [diff] [review]
Patch, v1

a1922=beltzner, man, I get nervous taking patches this big on the branch

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ae89af4f7cd0

Comment 9

[Jonathan Kew [:jfkthame]]

(In reply to comment #8)
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ae89af4f7cd0

This appears to have caused the persistent leak on Windows mochitests; see mozilla-1.9.2 tinderboxen:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1272 bytes during test execution (threshold set at 484 bytes)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 159 instances of nsStringBuffer with size 8 bytes each (1272 bytes total)

A similar persistent leak shows up on trunk builds on tryserver, though it doesn't get reported on the mozilla-central tinderboxes for some reason. I did a tryserver build with this patch reverted, and confirmed that the leak disappeared.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Leax fix

Yeah, I have no idea why this doesn't bite m-c. Some sort of shutdown ordering thing? Maybe we GC more on m-c than on branch? Who knows.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed leak fix:

http://hg.mozilla.org/mozilla-central/rev/59622926cd6b
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a459d11674b4