Last Comment Bug 647390 - Remove the calloc+placement-new pattern
: Remove the calloc+placement-new pattern
Status: RESOLVED FIXED
[mentor=pbiggar]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-01 18:52 PDT by Paul Biggar
Modified: 2011-09-29 15:52 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use OffTheBooks::new_ instead of the calloc+new-placement pattern. (6.75 KB, patch)
2011-09-27 20:23 PDT, Reuben Morais [:reuben]
luke: review+
Details | Diff | Review

Description Paul Biggar 2011-04-01 18:52:29 PDT
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.
Comment 1 Reuben Morais [:reuben] 2011-09-27 20:23:13 PDT
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.
Comment 2 Luke Wagner [:luke] 2011-09-28 08:56:27 PDT
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.
Comment 3 Luke Wagner [:luke] 2011-09-28 09:36:31 PDT
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?
Comment 4 Reuben Morais [:reuben] 2011-09-28 12:17:44 PDT
(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 Luke Wagner [:luke] 2011-09-28 13:43:05 PDT
Hm, crash on try server; investigating.
Comment 6 Luke Wagner [:luke] 2011-09-29 09:16:52 PDT
Ah, cx->thread_ needs to be initialized by the time ContextStack's constructor is called.

https://hg.mozilla.org/integration/mozilla-inbound/rev/611807062ab5
Comment 7 :Ehsan Akhgari (out sick) 2011-09-29 15:52:04 PDT
https://hg.mozilla.org/mozilla-central/rev/611807062ab5

Note You need to log in before you can comment on or make changes to this bug.