Don't allocate zero-sized script data.

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 8365030 [details] [diff] [review]
Don't allocate zero-sized script data.
Attachment #8365030 - Flags: review?(luke)
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
(Assignee)

Comment 3

5 years ago
Created attachment 8365035 [details] [diff] [review]
Don't allocate zero-sized script data.

And these things are never as easy as they first seem...
Attachment #8365035 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #8365030 - Attachment is obsolete: true
Attachment #8365030 - Flags: review?(luke)

Comment 4

5 years ago
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?
Whiteboard: [MemShrink]
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
Created attachment 8365899 [details] [diff] [review]
Don't allocate zero-sized script data.

Tweaking the assertion in PodCopy() appears to be enough to get this to work.
Attachment #8365899 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #8365035 - Attachment is obsolete: true
Attachment #8365035 - Flags: review?(luke)

Comment 7

5 years ago
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

5 years ago
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.
Attachment #8365899 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/42bb4abbd6c9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.