Closed Bug 647390 Opened 13 years ago Closed 13 years ago

Remove the calloc+placement-new pattern

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: paul.biggar, Unassigned)

Details

(Whiteboard: [mentor=pbiggar])

Attachments

(1 file)

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.
Whiteboard: [good first bug] → [mentor=pbiggar]
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 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+
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!
Hm, crash on try server; investigating.
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
https://hg.mozilla.org/mozilla-central/rev/611807062ab5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: