1499192 - Remove null-termination of ScriptSource chunks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Ted Campbell [:tcampbell]]

The patch in Bug 1493441 raises the question of why we have null-termination of chunks when we otherwise don't rely on null-termination in the engine. Looking through history it is hard to tell what the original reason was.

https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/js/src/vm/JSScript.cpp#1875

Comment 1

[Ted Campbell [:tcampbell]]

Luke, do you recall any of the history of source compression or null-terminated source in the engine? It seems like nothing relies on it today.

Comment 2

[Luke Wagner [:luke]]

I can't think of anything; maybe someone wanted to "play it safe"... but by all means feel free to remove if you can't see any reason not to.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

It looks like this goes all the way back to bug 761723.  That bug documents no reason for null termination.  It looks like this can go.

I lightly smoketested this on jstests.py of tests that match "toString".  No rush to tryserver this -- this isn't a patch I would land til we're out of a freezy period for sure, just in case.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 9017643 [details] [diff] [review]
Patch

Review of attachment 9017643 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Compression.h
@@ +78,2 @@
>      static size_t chunkSize(size_t uncompressedBytes, size_t chunk) {
> +        size_t lastChunk = uncompressedBytes / CHUNK_SIZE;

Hm is this really correct? If we have CHUNK_SIZE = 100 and uncompressedBytes = 100, then lastChunk used to be 0 because we need only one chunk but now it will be 1. Maybe the code below handles this correctly anyway but I think it would be nice to have the correct lastChunk value here for the assert below.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: v2

(In reply to Jan de Mooij [:jandem] from comment #4)
> >      static size_t chunkSize(size_t uncompressedBytes, size_t chunk) {
> > +        size_t lastChunk = uncompressedBytes / CHUNK_SIZE;
> 
> Hm is this really correct? If we have CHUNK_SIZE = 100 and uncompressedBytes
> = 100, then lastChunk used to be 0 because we need only one chunk but now it
> will be 1. Maybe the code below handles this correctly anyway but I think it
> would be nice to have the correct lastChunk value here for the assert below.

Hrm, maybe.  I think I was misreading what this was doing, in part because the algorithm is not expressed very readably.  This patch rewrites it to be a whole lot more understandable, and to get rid of that misleading-in-this-bug's-context subtract-one.

Comment 6

[Ted Campbell [:tcampbell]]

Does this need to change? https://searchfox.org/mozilla-central/source/js/src/vm/Compression.cpp#153

Comment 7

[Jeff Walden [:Waldo]]

Don't think so -- that's just a ceildiv, nothing about null termination.

Comment 8

[Ted Campbell [:tcampbell]]

Comment on attachment 9018019 [details] [diff] [review]
v2

Review of attachment 9018019 [details] [diff] [review]:
-----------------------------------------------------------------

Best I can tell, this looks right.

::: js/src/vm/Compression.h
@@ +89,1 @@
>      }

So much more readable :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee64821abef
Don't null-terminate ScriptSource source-code chunks.  r=tcampbell

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/eee64821abef