Remove the calloc+placement-new pattern

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Paul Biggar, Unassigned)

Tracking

Trunk
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

([mentor=pbiggar])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
A reasonably common pattern I see is:

    void *mem = OffTheBooks::calloc_(sizeof *cx);
    if (!mem)
        return NULL;

    cx = new (mem) JSContext(rt);

These would be better expresed as:

    cx = OffTheBooks::new_<JSContext>(rt);

The difference between the two forms is that the latter doesnt initialize all fields to zero, so NULL initializers should be added the the constructors of those classes (also useful for avoiding OOMs).

An alternative would be to add a newc_ function to the allocators, which clears and allocates. I prefer the former, but this will do if there is some technical reason it can't land.
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug] → [mentor=pbiggar]
Created attachment 562963 [details] [diff] [review]
Use OffTheBooks::new_ instead of the calloc+new-placement pattern.

I'm not sure if this is still relevant, I could only find three occurrences of the mentioned pattern in the tree, but here is a patch anyway :)

I'd rather go with the newc_ alternative here, since the initialization lists required for this change are large and not very meaningful, and need to be changed whenever a new member is added to those classes.

However, those are not really technical reasons for it not to land, so this patch uses new_<T> and adds all missing members to the initialization lists of JSContext, JSRuntime and its member struct GCData.
Attachment #562963 - Flags: review?
Attachment #562963 - Flags: review? → review?(paul.biggar)
Attachment #562963 - Flags: review?(paul.biggar) → review?(luke)

Comment 2

6 years ago
Comment on attachment 562963 [details] [diff] [review]
Use OffTheBooks::new_ instead of the calloc+new-placement pattern.

>-    void *mem = OffTheBooks::calloc_(sizeof(JSRuntime));
>-    if (!mem)
>-        return NULL;
>-
>-    JSRuntime *rt = new (mem) JSRuntime();
>+    JSRuntime *rt = OffTheBooks::new_<JSRuntime>();
>     if (!rt->init(maxbytes)) {

Still need the OOM check.

>-    void *mem = OffTheBooks::calloc_(sizeof *cx);
>-    if (!mem)
>-        return NULL;
>-
>-    cx = new (mem) JSContext(rt);
>+    cx = OffTheBooks::new_<JSContext>(rt);
>     cx->debugHooks = &rt->globalDebugHooks;

Here too.
Attachment #562963 - Flags: review?(luke) → review+

Comment 3

6 years ago
I had to rebase and rearrange a few things to get rid of compiler warnings.  Want me to land the patch with these fixes for you?
(In reply to Luke Wagner [:luke] from comment #3)
> I had to rebase and rearrange a few things to get rid of compiler warnings. 
> Want me to land the patch with these fixes for you?

Sure, thanks for the review!

Comment 5

6 years ago
Hm, crash on try server; investigating.

Comment 6

6 years ago
Ah, cx->thread_ needs to be initialized by the time ContextStack's constructor is called.

https://hg.mozilla.org/integration/mozilla-inbound/rev/611807062ab5
Target Milestone: --- → mozilla10

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/611807062ab5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.