963528 - Don't allocate zero-sized script data.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

We have lots of JSScripts that have zero-sized data (roughly 20--25% of them),
for which we call calloc(0). On Linux, jemalloc allocates one word for a such a
request.

On 64-bit platforms, this accounts for about 30 KiB of memory at start-up, and
40 KiB with a few tabs open. On 32-bit it'll be half that. Chump change for
desktop, but worth a tiny change for B2G.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Don't allocate zero-sized script data.

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8365030 [details] [diff] [review]
Don't allocate zero-sized script data.

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

::: js/src/jsscript.cpp
@@ +1938,5 @@
> +        if (!script->data)
> +            return false;
> +    } else {
> +        script->data = nullptr;
> +    }

it looks like you should be able to change this to
size_t size = ...
script->dataSize_ = size;
if (size == 0) {
    script->data = nullptr;
    return true;
}

Everything after this should only have an effect if size > 0

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Don't allocate zero-sized script data.

And these things are never as easy as they first seem...

Comment 4

[Luke Wagner [:luke]]

Could you instead leave script->data null in the zero-length case?  Seems like it might catch various bugs where people attempted to index into script->data when they shouldn't?

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Luke Wagner [:luke] from comment #4)
> Could you instead leave script->data null in the zero-length case?  Seems
> like it might catch various bugs where people attempted to index into
> script->data when they shouldn't?

I tried that and hit various assertion failures; things like a PodCopy (relating to Bindings) complaining about src==dst. I guess I could try to fix those instead. That would allow the extra test upon freeing to disappear, and I also missed a similar test in sizeOfData() that wouldn't be necessary with null.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Don't allocate zero-sized script data.

Tweaking the assertion in PodCopy() appears to be enough to get this to work.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8365899 [details] [diff] [review]
Don't allocate zero-sized script data.

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

So I guess you *could* tweak this assertion, because the implementation of PodCopy doesn't actually complain about "overlapping" memory regions when zero elements are to be copied.  But PodCopy's interface says you're not supposed to do this, regardless.  Given PodCopy was basically supposed to be an infer-length-from-count-and-type-size safe version of memset, I'm not sure I like this change.  I'd rather see the if-zero checks at the callers.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8365899 [details] [diff] [review]
Don't allocate zero-sized script data.

The Googles seem to agree that calling memcpy(null, null, 0) is invalid per spec.  Surely no implementation will actually crash, but it does seem more proper to put the null check in the caller.

Comment 9

[Nicholas Nethercote [inactive]]

Landed with the check in the caller:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42bb4abbd6c9

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42bb4abbd6c9