NewCompartment should account memory using gcMallocBytes

RESOLVED FIXED in mozilla2.0

Status

()

RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: gal, Assigned: paul.biggar)

Tracking

(Blocks: 2 bugs, {memory-footprint})

Trunk
mozilla2.0
memory-footprint
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status2.0 wanted)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Otherwise we don't GC when making a lot of sandboxes (or compartments in general).
(Reporter)

Comment 1

8 years ago
This is trivial. I would take a reviewed patch but not blocking on it.
status2.0: --- → wanted
(Reporter)

Updated

8 years ago
Assignee: general → pbiggar
(Reporter)

Comment 2

8 years ago
Paul want to take a stab at this? You and nick were looking at the allocation patterns in the engine (js_new<> and such).
Blocks: 632234
Created attachment 512626 [details] [diff] [review]
patch

Is this on the right track?

NewCompartment() currently has two "return NULL" paths in which the allocated compartment is not freed.  That looks wrong.
Attachment #512626 - Flags: review?(gal)
(Assignee)

Comment 4

8 years ago
Sure, I'll take it over, if njn doesn't mind. Is njn's patch all you want, or is there more memory that isn't being accounted?

(I'll add the OOM checking too.)
It's all yours!
(Reporter)

Comment 6

8 years ago
Thanks to both of you. I think JSCompartment is the much smaller part. We also should account for the tracer and JM and the property tree.
(Reporter)

Comment 7

8 years ago
(basically all the js_new stuff is not accounted for right now)
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> (basically all the js_new stuff is not accounted for right now)

Ah, of course. So js_new etc calls js_malloc directly, instead of calling cx->malloc?
(Reporter)

Comment 9

8 years ago
Yeah
(Assignee)

Comment 10

8 years ago
(In reply to comment #8)
> (In reply to comment #7)
> > (basically all the js_new stuff is not accounted for right now)
> 
> Ah, of course. So js_new etc calls js_malloc directly, instead of calling
> cx->malloc?

Here's roughly my plan. Thoughts njn?

-#define JS_NEW_BODY(t, parms)                                                 \
-    void *memory = js_malloc(sizeof(t));                                      \
+#define JS_NEW_BODY(t, parms, cx)                                             \
+    void *memory = cx ? cx->malloc(sizeof(t)) : js_malloc(sizeof(t));         \
     return memory ? new(memory) t parms : NULL;

 template <class T>
-JS_ALWAYS_INLINE T *js_new() {
+JS_ALWAYS_INLINE T *js_new(JSContext *cx = NULL) {
     JS_NEW_BODY(T, ())
 }
I guess so, though I'll need to see a real patch to really understand.  In what cases would you pass NULL to js_new()?
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> I guess so, though I'll need to see a real patch to really understand. In what
> cases would you pass NULL to js_new()?

If there wasn't a convenient JSContext (for example, when we're deep in Yarr).
(Assignee)

Comment 13

8 years ago
Created attachment 512804 [details] [diff] [review]
WIP 1

This doesn't work, but is most of the way there.

Basically, I'm replacing:

js_new -> cx->create
js_delete -> cx->destroy
js_malloc -> cx->malloc
js_free -> cx->free

etc.

If there is no cx, I use a JSRuntime.

Things are a bit weird when we're creating the first Compartment, so WIP. Also, I have to hunt down many more js_xs, and write some |make check| script to prevent regression.
Attachment #512626 - Attachment is obsolete: true
Attachment #512626 - Flags: review?(gal)
(In reply to comment #13)
> 
> js_new -> cx->create
> js_delete -> cx->destroy
> js_malloc -> cx->malloc
> js_free -> cx->free

I came to the conclusion overnight that the other way is better:

  cx->malloc(n)   --> js_malloc(cx, n)
  cx->free(p)     --> js_free(p)
  js_new<T>(...)  --> js_new<T>(cx, ...)
  js_delete<T>(p) --> (unchanged)

I've always found |cx->malloc| confusing.  It looks like it allocates in a per-context pool or something, but it doesn't.  It just uses |cx| as a normal argument, just like a zillion other functions in SM, so why not pass it as a normal parameter?  We even get to use the standard SM idiom where "cx as first param means the function is fallible".  And unlike cx->malloc, it also works in the few places where we don't have access to a |cx| -- you can pass in NULL.

We could also have js_malloc/js_new alternatives that take a runtime instead of a cx, if that's helpful.

The non-cx, non-runtime version of js_malloc() (ie. the base allocator) would still be present, but should only called from the cx and runtime versions of js_malloc/js_new.  It would be nice to auto-check that this is true.
(Assignee)

Comment 15

8 years ago
(In reply to comment #14)
> (In reply to comment #13)
> > 
> > js_new -> cx->create
> > js_delete -> cx->destroy
> > js_malloc -> cx->malloc
> > js_free -> cx->free
> 
> I came to the conclusion overnight that the other way is better:
> 
>   cx->malloc(n)   --> js_malloc(cx, n)
>   cx->free(p)     --> js_free(p)
>   js_new<T>(...)  --> js_new<T>(cx, ...)
>   js_delete<T>(p) --> (unchanged)

I came to a similar conclusion, largely because sometimes you need to delete from a destructor, and there isn't a context in sight.
(Assignee)

Comment 16

8 years ago
(In reply to comment #14)
> (In reply to comment #13)
> > 
> > js_new -> cx->create
> > js_delete -> cx->destroy
> > js_malloc -> cx->malloc
> > js_free -> cx->free
> 
> I came to the conclusion overnight that the other way is better:
> 
>   cx->malloc(n)   --> js_malloc(cx, n)
>   cx->free(p)     --> js_free(p)
>   js_new<T>(...)  --> js_new<T>(cx, ...)
>   js_delete<T>(p) --> (unchanged)

Well this is unpleasant. cx->create and cx->destroy have access to pricate constructors because the constructed class is a friend of JSContext. I can't figure out how to do that for global functions, and njn's comment from jsutil.h indicates it's more trouble than it's worth:

> * If you have a class with a private constructor or destructor, you can
> * make js_new/js_delete a friend.  This can be fiddly, and the interaction of
> * template functions, friend functions and namespaces can overwhelm even
> * modern compilers.  Manual inlining is probably easier.

Am currently trying a special class wrapper to js_new to explicitly be a friend. You would call AllocatorFriend::js_new<T> and |friend ::AllocatorFriend|.
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
> >   js_new<T>(...)  --> js_new<T>(cx, ...)
> >   js_delete<T>(p) --> (unchanged)

And another new plan. The one above is really unpleasant because some functions already take a JSRuntime or JSContext as their first parameter, and I can't figure out any way to make it hard to do that. Another alternative is make js_new a macro:

#define js_new(alloc, type, ...) __js_new<type>(alloc, __VA_ARGS__)

which works, but is unclear and makes error messages very unpleasant.

jorendorff suggestd (and also suggested the macro thing above) something like:

OffTheBooks::js_new<type>()
cx->js_new<type>
rt->js_new<type>

which is currently our best option I think.
(Assignee)

Comment 18

8 years ago
Are we allowed change the interface of js_malloc. It's lowercase, so it should be private, but the jscustomallocator.h stuff worries me.
(Assignee)

Comment 19

8 years ago
Created attachment 514596 [details] [diff] [review]
finished patch

This gives 3 standard ways of allocating memory:
 - using a context
 - using a runtime
 - using OffTheBooks

each of which has the same functions:

 - ->js_new
 - ->malloc
 - ->calloc
 - ->realloc
 - ->array_new

Freeing memory is normal:
 - ::js_delete
 - ::js_free
 - ::js_array_delete


I've replaced all calls to js_new or js_malloc with calls through these interfaces. Whenever there was a cx or rt available, that was used. When there was nothing, OffTheBooks was used. This makes it nice and simple to know when memory is being allocated without a runtime being told: grep for OffTheBooks.

This also got around the private constructors problem: it's easy and portable to friend a class (OffTheBooks), while it was not for template functions (js_new). So I took out all the inlined js_new() calls (except ones that used calloc rather than malloc).

I reordered the allocation of atomsCompartment so that the GC is inited first, which should solve the original problem. I also deleted the extra compartments on OOM.

The only thing this patch doesn't do that I'd like to is to remove ::js_malloc, ::js_calloc and ::js_realloc, but I think I can't because of allowing custom allocators.
Attachment #512804 - Attachment is obsolete: true
Attachment #514596 - Flags: review?(nnethercote)
Comment on attachment 514596 [details] [diff] [review]
finished patch

Nice patch, it looks like a pretty clean solution to a tricky problem.  Two
thumbs up for getting those private constructors/destructors to work nicely.
And I like the name "OffTheBooks" :)


>  - ->js_new
>  - ->malloc
>  - ->calloc
>  - ->realloc
>  - ->array_new
> 
> Freeing memory is normal:
>  - ::js_delete
>  - ::js_free
>  - ::js_array_delete

It's a shame |js_new| can't be called |new|.  I'll suggest adding |js_|
prefixes to malloc/calloc/realloc/array_new for consistency, but I'll leave
the final decision up to you.


>-    if (!(atomsCompartment = js_new<JSCompartment>(this)) ||
>+    if (!js_InitGC(this, maxbytes))
>+        return false;
>+
>+    if (!(atomsCompartment = this->js_new<JSCompartment>(this)) ||
>         !atomsCompartment->init() ||
>         !compartments.append(atomsCompartment)) {
>         return false;
>     }

If init() or append() fails, won't the compartment have leaked?

>-    if (!js_InitGC(this, maxbytes) || !js_InitAtomState(this))
>+    atomsCompartment->setGCLastBytes(8192);
>+
>+    if (!js_InitAtomState(this))
>         return false;

Same here?  And maybe more like this further down in this function?


>diff --git a/js/src/jsarena.cpp b/js/src/jsarena.cpp
>--- a/js/src/jsarena.cpp
>+++ b/js/src/jsarena.cpp
>@@ -155,22 +155,22 @@ JS_ArenaAllocate(JSArenaPool *pool, size
>-                b = (JSArena *) js_malloc(gross);
>+                b = (JSArena *) js::OffTheBooks::malloc(gross);
>                 if (!b)

Arenas can allocate a lot of memory.  It would be nice to get them on the
books.  Can you try adding a JSContext* to JSArenaPool to get it on the
books?  It may end up being a pain, but seems worthwhile to try.

Likewise the js_calloc() in nanojit::Allocator::allocChunk -- could add a
JSContext* to VMAllocator.  Likewise for Tracker.  In general, every
OffTheBooks allocation is undesirable, right?  Maybe you can file a
follow-up bug to reduce them further?  (If so, you could add a "FIXME: bug
<NNNNNN>" comment to the comment explaining OffTheBooks.)


>+    CHECK_MALLOC(dataReserve, char*, DataReserveSize);
>+    CHECK_MALLOC(traceReserve, char*, TraceReserveSize);
>+    CHECK_MALLOC(tempReserve, char*, TempReserveSize);

Those allocations are reasonably big and easy to get on the books -- can you
change?


>+/*
>+ * Note lack of ; in JSRunttime below. This is intentional so "calling" this
>+ * looks "normal".
>+ */

Nit: s/JSRunttime/JSRuntime/
Attachment #514596 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 21

8 years ago
Created attachment 514651 [details] [diff] [review]
Add checking script

This adds a checking script to prevent regressions, and fixes the few mistakes it found. The checking script is run upon |make check| and it checks that someone doesn't add new calls to js_malloc, js_calloc, js_realloc or OffTheBooks.

More calls to OffTheBooks are expected, in which case |make check| will fail, and Makefile.in will need to be updated.
Attachment #514596 - Attachment is obsolete: true
Attachment #514651 - Flags: review?(nnethercote)
(Assignee)

Comment 22

8 years ago
Comment on attachment 514651 [details] [diff] [review]
Add checking script

Didn't see your response there.
Attachment #514651 - Flags: review?(nnethercote)

Comment 23

8 years ago
Nice work.  I also mourn the verbosity of cx->js_new but I also don't see a
better alternative.  My one nit is: could you drop the js:: in
js::OffTheBooks::js_new for uses in .cpp's?  Those should be in scope of a
'using namespace js'.
(Reporter)

Comment 24

8 years ago
Please also see bug 636300 and the background thread delete I need there.
Attachment #514651 - Flags: review+
(Assignee)

Comment 25

8 years ago
(In reply to comment #20)

> And I like the name "OffTheBooks" :)

Me too, but I can't claim credit - jorendorff came up with it.


> It's a shame |js_new| can't be called |new|.  I'll suggest adding |js_|
> prefixes to malloc/calloc/realloc/array_new for consistency, but I'll leave
> the final decision up to you.

I agree, I'd like to use |new|. I'm not particularly tied to leaving off the js_, but the script I added to the recent patch does take advantage of it.

Any thoughts on cx->NEW?


> >-    if (!js_InitGC(this, maxbytes) || !js_InitAtomState(this))
> >+    atomsCompartment->setGCLastBytes(8192);
> >+
> >+    if (!js_InitAtomState(this))
> >         return false;
> 
> Same here?  And maybe more like this further down in this function?

I don't think so. After append succeeds, atomCompartment's cleanup should be handled by |compartments|. I assume this is cleaned up somewhere, though I can't find it at first glance.
 

> Arenas can allocate a lot of memory.  It would be nice to get them on the
> books.  Can you try adding a JSContext* to JSArenaPool to get it on the
> books?  It may end up being a pain, but seems worthwhile to try.

The arena allocation site is part of the JSAPI. Is it worth changing? If so, it's not too hard to do.
(Assignee)

Comment 26

8 years ago
(In reply to comment #24)
> Please also see bug 636300 and the background thread delete I need there.

I think you're saying that I've converted some cx->destry and cx->free calls into js_delete and js_free which aren't freed in the background?
(Assignee)

Comment 27

8 years ago
(In reply to comment #23)
> Nice work.  I also mourn the verbosity of cx->js_new but I also don't see a
> better alternative.  My one nit is: could you drop the js:: in
> js::OffTheBooks::js_new for uses in .cpp's?  Those should be in scope of a
> 'using namespace js'.

How do you feel about cx->New? It seems legal...
> The arena allocation site is part of the JSAPI. Is it worth changing? If so,
> it's not too hard to do.

I'm not sure what you mean... Arenas aren't part of the JSAPI, AFAICT.

> How do you feel about cx->New? It seems legal...

I was just about to suggest it! :)
(Assignee)

Comment 29

8 years ago
(In reply to comment #28)
> > The arena allocation site is part of the JSAPI. Is it worth changing? If so,
> > it's not too hard to do.
> 
> I'm not sure what you mean... Arenas aren't part of the JSAPI, AFAICT.

extern JS_PUBLIC_API(void)
JS_InitArenaPool(JSArenaPool *pool, const char *name, size_t size,
                 size_t align, size_t *quotap);


I thought the JSAPI was marked by JS_PUBLIC_API macros, but I could be wrong about this.
(In reply to comment #29)
> 
> extern JS_PUBLIC_API(void)
> JS_InitArenaPool(JSArenaPool *pool, const char *name, size_t size,
>                  size_t align, size_t *quotap);

Oh.  But jsarena.h doesn't seem to be included by jsapi.h.  I suggest you leave it for now and come back to it in the follow-up bug.
(Assignee)

Comment 31

8 years ago
(In reply to comment #26)
> (In reply to comment #24)
> > Please also see bug 636300 and the background thread delete I need there.

I'm currently replacing js_delete with cx->Delete and Foreground::Delete.

I see rt->gcHelperThread. Should I add rt->free() and rt->Delete??
(In reply to comment #31)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > Please also see bug 636300 and the background thread delete I need there.
> 
> I'm currently replacing js_delete with cx->Delete and Foreground::Delete.

This makes me a little nervous -- what happens if mismatches occur?  Presumably the GC accounting gets screwed up.  Is there some way to check for mismatches in debug mode?
(Assignee)

Comment 33

8 years ago
(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #26)
> > > (In reply to comment #24)
> > > > Please also see bug 636300 and the background thread delete I need there.
> > 
> > I'm currently replacing js_delete with cx->Delete and Foreground::Delete.
> 
> This makes me a little nervous -- what happens if mismatches occur?

The helper thread keeps a vector of memory it has to free, and then frees it using js_free, but it doesn't touch accounting at all. A mismatch will work fine: the memory will be freed in the background of a different thread. However, I can think of a pathological case:

 - context A does lots of allocation
 - context B does no allocation, but frees a lot
 - context B free's thread A's allocations
 - context A gets collected a lot, but has nothing to free
 - context B never gets collected, and so holds on to its entire free list.


>  Is there some way to check for mismatches
> in debug mode?

I'm not sure. Mismatches are superficially OK, so we might get false positives. But I can always try.
(In reply to comment #33)
> > 
> > This makes me a little nervous -- what happens if mismatches occur?
> 
> The helper thread keeps a vector of memory it has to free, and then frees it
> using js_free, but it doesn't touch accounting at all.

Oh, right.  That's not so bad then.  If it's easy to detect mismatches, that would be nice, otherwise just a comment somewhere about this would be good.
Capitalizing method names goes against style we have in common with JSC and nanojit. It also looks funny for common Unix libc names such as malloc and free (is the latter prefixed with js_ because of mozalloc.h?).

I can't say js_ prefixes all around beats capitalization -- my reaction is "a pox on both those houses". I'd much rather we use trialing _ only for C++ keywords and plain names for the rest. Reactions?

/be
(Assignee)

Comment 36

8 years ago
(In reply to comment #35)
> Capitalizing method names goes against style we have in common with JSC and
> nanojit. It also looks funny for common Unix libc names such as malloc and free
> (is the latter prefixed with js_ because of mozalloc.h?).

It's not prefixed in method calls. It's prefixed in jsutil.h for lots of reasons, mozalloc is one of them, LD_PRELOAD is another, allowing embedding is another.


> I can't say js_ prefixes all around beats capitalization -- my reaction is "a
> pox on both those houses". I'd much rather we use trialing _ only for C++
> keywords and plain names for the rest. Reactions?

Functionally, they only difference is that the capital versions are easier to code-complete; not a big deal though. Aesthetically, I prefer capitalization, but I think it's a pox all round.

I do prefer any of these options to cx->create and cx->destroy which is what's currently there - I would never have guessed they were purely allocation functions.

Comment 37

8 years ago
(In reply to comment #36)
> I do prefer any of these options to cx->create and cx->destroy which is what's
> currently there - I would never have guessed they were purely allocation
> functions.

At the time, SM hackers polled on #jsapi thought "create"/"destroy" were the most logical names given that 'new' wasn't an option and, as we have now seen thrice, the various manglings of 'new' strike people as unattractive.   Also, given that the first sentence of the C++ standard section 5.3.4 "New" begins:
  "The new-expression attempts to create an object of the type-id ..."
the names don't seem too far off the mark.
(In reply to comment #37)
>  Also,
> given that the first sentence of the C++ standard section 5.3.4 "New" begins:
>   "The new-expression attempts to create an object of the type-id ..."
> the names don't seem too far off the mark.

Not all of us have sections of the C++ standard memorized! :P

I prefer 'New', '_new' and 'js_new' to 'create'.
njn: how about new_?

All: the create/destroy pair is less good if higher-level than memory allocation and release, since create implies new + init and destroy is finish + delete/free. If we're adding thing veneer to C stdlib and C++ operators, we should stick as close to the canonical names as we can.

In my view, that means trailing _ beats capitalization. But it's a close call.

/be
'new_' is fine by me.
(Assignee)

Comment 41

8 years ago
New is used in some places already, each of which do more than simple memory allocation: MonoIC, JSWrapper, CTypes. So I guess new_ is best, even if it is unsightly.
The trailing underscore can be squinted at hard, to make it look like a space (but the mandatory parens around the arglist remain).

Same for delete_ then?

/be
s/thing veneer/thin veneer/ in comment 39.

/be
(Assignee)

Updated

8 years ago
Blocks: 636558
(Assignee)

Updated

8 years ago
Blocks: 636561
(Assignee)

Comment 44

8 years ago
Created attachment 514921 [details] [diff] [review]
Near final patch

This is the final version I think.

However, I'm slightly worried by bug 636561 comment 3 and bug 636558 comment 5, since this does a small portion of what they're talking about, so I'm getting one more review?

Gal, do we want this for FF4?
Attachment #514651 - Attachment is obsolete: true
Attachment #514921 - Flags: review?(gal)
(Reporter)

Comment 45

8 years ago
Nice patch. I think we can survive without this in FF4. We have 3 bugs targeting the same issue from different angles and one of them is already in. So lets land this right after FF4 branches.
(Assignee)

Comment 46

8 years ago
Created attachment 517216 [details] [diff] [review]
Final patch

This is the final patch I'm planning to push on Monday, unless there is objection. It differs from the old patch in that I divided Foreground into two classes: Foreground and UnwantedForeground.

With the last patch, there was a |make check| rule indicating that we wanted to get rid of Foreground deallocations. However, that wasn't really true (see bug 636561 comment 3 and bug 636561 comment 5). We only want to get rid of Foreground deallocations on the fast path, hence UnwantedForeground.

So now deallocations in the foreground which we don't want use UnwantedForeground, and deallocations in the foreground which we do want use Foreground.
Attachment #514921 - Attachment is obsolete: true
Attachment #514921 - Flags: review?(gal)
Blocks: 640452
Comment on attachment 517216 [details] [diff] [review]
Final patch

>diff --git a/js/src/bench.sh b/js/src/bench.sh
>old mode 100755
>new mode 100644

?

>diff --git a/js/src/config/find_vanilla_new_calls b/js/src/config/find_vanilla_new_calls
>--- a/js/src/config/find_vanilla_new_calls
>+++ b/js/src/config/find_vanilla_new_calls
>@@ -14,17 +14,17 @@
> #   alloc/free checking.
> # - Doesn't detect the 'nothrow' variants, which are ok but probably still
> #   best avoided.
> # - Is designed to only run on Linux (though it may also work on Mac);  one
> #   platform will be enough to catch any violations.
> #
> # If this script fails:
> # - You need to find the uses of vanilla new/delete and replace them with
>-#   js_new()/js_delete().
>+#   {js::Foreground,JSContext,JSRuntime}::{New,/array_new}.

'New' or 'new_'?

>diff --git a/js/src/jsutil.h b/js/src/jsutil.h
>--- a/js/src/jsutil.h
>+++ b/js/src/jsutil.h
>+ *   - Classes which have private constructors or destructors should have
>+ *     JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR added to their
>+ *     declaration (with no ';').

>+ *   JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR makes the allocation
>+ *   classes friends with your class, giving them access to private
>+ *   constructors and destructors. Adding a superfluous ';' after the
>+ *   declaration will cause the Mozilla tinderbox to go red, so avoid it.

>+/*
>+ * Note lack of ; in JSRuntime below. This is intentional so "calling" this
>+ * looks "normal".
>+ */
>+#define JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR \
>+    friend class js::OffTheBooks;\
>+    friend class js::Foreground;\
>+    friend class js::UnwantedForeground;\
>+    friend struct ::JSContext;\
>+    friend struct ::JSRuntime

>diff --git a/js/src/yarr/yarr/RegexPattern.h b/js/src/yarr/yarr/RegexPattern.h
>--- a/js/src/yarr/yarr/RegexPattern.h
>+++ b/js/src/yarr/yarr/RegexPattern.h
> struct CharacterClassTable {
>+
>+    JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR;
>+

So, what's up here?
(Assignee)

Comment 48

8 years ago
Created attachment 518859 [details] [diff] [review]
near final patch

(In reply to comment #47)
> >diff --git a/js/src/bench.sh b/js/src/bench.sh
> >old mode 100755
> >new mode 100644
> ?

Indeed. Dunno how that snuck in.



> >-#   js_new()/js_delete().
> >+#   {js::Foreground,JSContext,JSRuntime}::{New,/array_new}.
> 
> 'New' or 'new_'?

new_. Fixed.

 
> >+ *     JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR added to their
> >+ *     declaration (with no ';').
> >+ * Note lack of ; in JSRuntime below. This is intentional so "calling" this
> >+ * looks "normal".
> >+ */
> >+#define JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR \
> >+    friend struct ::JSRuntime
> > struct CharacterClassTable {
> >+
> >+    JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR;
> >+
> 
> So, what's up here?

Error in the comment. The ';' isn't consumer facing, so I've removed it.


Great catches, thanks.
Attachment #517216 - Attachment is obsolete: true
(Assignee)

Comment 49

8 years ago
cjones, I can't figure this out. This patch causes the build to break at various points related to moz_free. For example:

g++ -o HttpBaseChannel.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Darwin10.6.0\" -DOSARCH=Darwin -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD  -DOS_MACOSX=1 -DOS_POSIX=1  -DIMPL_NS_NET -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/../../base/src -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/xpcom/ds -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/content/base/src -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/content/events/src  -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/ipc/chromium/src -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/obj-x86_64-apple-darwin10.6.0/dist/include/nspr -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/obj-x86_64-apple-darwin10.6.0/dist/include/nss       -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -gstabs -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/HttpBaseChannel.pp /Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/HttpBaseChannel.cpp
In file included from ../../../ipc/ipdl/_ipdlheaders/mozilla/net/PHttpChannel.h:21,
                 from ../../../ipc/ipdl/_ipdlheaders/mozilla/net/PHttpChannelChild.h:10,
                 from ../../../dist/include/mozilla/net/HttpChannelChild.h:48,
                 from /Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:108:
../../../dist/include/mozilla/net/NeckoMessageUtils.h: In static member function ‘static void IPC::ParamTraits<IPC::InputStream>::Write(IPC::Message*, const IPC::InputStream&)’:
../../../dist/include/mozilla/net/NeckoMessageUtils.h:218: warning: unused variable ‘rv’
../../../dist/include/mozilla/net/NeckoMessageUtils.h:229: warning: unused variable ‘rv’
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp: In function ‘nsresult PrepareAcceptLanguages(const char*, nsACString_internal&)’:
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:1191: error: ‘moz_free’ is not a member of ‘nsCRT’
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:1222: error: ‘moz_free’ is not a member of ‘nsCRT’
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp: In function ‘nsresult PrepareAcceptCharsets(const char*, nsACString_internal&)’:
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:1349: error: ‘moz_free’ is not a member of ‘nsCRT’

I had other problems with moz_strdup when I sprinkled the headers more.

I note that xpcom/base/nscore.h opens macro_wrappers but doesn't close them, is that right? Likewise in ipc/chromium/src/base/message_pump_qt.h, but I doubt that's our problem here.

What are the rules for using macro wrappers? Can you enumerate them, and I'll add them as a comment somewhere appropriate?
The goal for the wrappers is to #define malloc/free/et al. for gecko; that's why nscore.h includes the header.  There aren't any hard-and-fast rules for including them, just whatever works (it's a big gross hack).  It would help understand what's failing if you made a preprocessed version of nsHttpHandler, |make -C $objdir/netwerk/protocol/http nsHttpHandler.i|.  If the macros are conflicting with JS headers, moving the JS includes before Gecko stuff can work around that.
(Assignee)

Updated

8 years ago
Depends on: 643548
(Assignee)

Comment 51

8 years ago
http://hg.mozilla.org/tracemonkey/rev/f949c8533aa0
Whiteboard: [fixed-in-tracemonkey]
Target Milestone: --- → mozilla2.0
Version: unspecified → Trunk
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Keywords: footprint
Blocks: 659855
Blocks: 659856
No longer blocks: 632234
No longer blocks: 640452
You need to log in before you can comment on or make changes to this bug.