890238 - Memory reporter for ICU

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

http://www.icu-project.org/apiref/icu4c/uclean_8h.html#a5dea766c5e726833400dc0ee24a2bc6a has details on ICU's u_setMemoryFunctions() function, which we can use to implement a memory reporter for ICU.

Comment 1

[Nicholas Nethercote [inactive]]

I enabled ICU by modifying configure.in and did a DMD run, but I'm not seeing any unreported allocations from ICU.  Do I need to visit a page that runs some particular JS code for ICU to be loaded, or something like that?

Comment 2

[Norbert Lindenberg]

You need to call some functionality in the ECMAScript Internationalization API. Running the conformance test suite will do that:
http://test262.ecmascript.org/testcases_intl402.html

Comment 3

[Nicholas Nethercote [inactive]]

I've got an ICU-enabled build working and I have some preliminary measurements from DMD that suggest the memory overhead after running the test262 tests is less than 200 KiB, which is good.

I'm having trouble working out where I should call u_setMemoryFunctions(), i.e. where ICU is first called/initialized.  js_InitIntlClass() is my best guess at the moment...

Comment 4

[Nicholas Nethercote [inactive]]

Another thought:  if ICU is called/initialized/loaded/whatever via JS, what happens in the presence of multiple JS runtimes?  This happens in the browser -- we have one main runtime, and then one additional runtime per web worker.

Comment 5

[Norbert Lindenberg]

Currently, ICU is only used by Intl objects, so js_InitIntlClass should work. js_InitIntlClass is called for each JS global object where Intl is used (which could be on different threads), so you have to make sure that u_setMemoryFunctions is called only once. However, ICU may in the future also be used for other purposes - see bug 724529.

Better might be to call u_setMemoryFunctions from the main application thread. If you plan to use u_cleanup() as well, that'll have to be on that thread anyway.

More information:
http://userguide.icu-project.org/design#TOC-ICU-Initialization-and-Termination

Comment 6

[Jeff Walden [:Waldo]]

Yeah, I was thinking about the multiple-runtime issue too, wrt bug 890127.  I think at one point there might have been a JS_Init() paired with a JS_ShutDown() (the latter still exists).  Perhaps we should reintroduce those to simply centralize this.  Right now I imagine it's not entirely trivial finding the one central place for it to be called, unless you did a JS_CallOnce-style hackaround.

Comment 7

[Terrence Cole [:terrence]]

What about in this block that runs once-per-process?

http://dxr.allizom.org/mozilla-central/source/js/src/jsapi.cpp#l1086

Comment 8

[Jeff Walden [:Waldo]]

That's kind of abominable, and it's not thread-safe against multiple threads calling JS_NewRuntime at once.  But it would work if we're ignoring those issues.  :-)  (How has tsan not complained about that field yet?)

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Add a memory reporter for ICU.

This patch implements a memory reporter for ICU.

Results
-------
On starting the browser, "explicit/icu" is 200 KB.  After running the intl402
tests, it climbs to 270 KB.

The smallness here surprised me.  I thought ICU had MBs of big tables.  Or are
they statically allocated?

API Design
----------
The main thing I'm unsure about this is the API.  I currently have a
JS_SetICUMemoryFunctions() function, which works but feels clunky.

Another possibility is to remove that function and just call
u_setMemoryFunctions() directly from XPCJSRuntime().  This would be simpler but
it would also require me to #include unicode/{utypes.h,uclean.h} from
XPCJSRuntime.cpp, which requires ENABLE_INTL_API, which requires js-confdefs.h,
which I think xpconnect cannot see?  Not sure.

Comment 10

[Norbert Lindenberg]

Comment on attachment 773097 [details] [diff] [review]
Add a memory reporter for ICU.

Review of attachment 773097 [details] [diff] [review]:
-----------------------------------------------------------------

I'm only looking at ICU usage, not at how and where this fits into JavaScript.

::: js/src/jsapi.cpp
@@ +1154,5 @@
> +                         JS_ICUFreeFn freeFn)
> +{
> +    UErrorCode status = U_ZERO_ERROR;
> +    u_setMemoryFunctions(context, allocFn, reallocFn, freeFn, &status);
> +    return status == U_ZERO_ERROR;

ICU can return both errors and warnings through status. Comparing with U_ZERO_ERROR means that warnings are treated as errors. More common ICU usage would be to return U_SUCCESS(status), which ignores warnings.

::: js/src/jsapi.h
@@ +1760,5 @@
>  JS_DestroyRuntime(JSRuntime *rt);
>  
> +typedef void *(*JS_ICUAllocFn)(const void *context, size_t size);
> +typedef void *(*JS_ICUReallocFn)(const void *context, void *p, size_t size);
> +typedef void (*JS_ICUFreeFn)(const void *context, void *p);

I assume these declarations exist to avoid clients having to include ICU headers. There should be a comment indicating that they must match those of UMemAllocFn, UMemReallocFn, UMemFreeFn in ICU's uclean.h header file.

@@ +1764,5 @@
> +typedef void (*JS_ICUFreeFn)(const void *context, void *p);
> +
> +extern JS_PUBLIC_API(bool)
> +JS_SetICUMemoryFunctions(const void *context, JS_ICUAllocFn allocFn, JS_ICUReallocFn reallocFn,
> +                         JS_ICUFreeFn freeFn);

The ICU header file uclean.h has stern warnings "Do not use unless you know what you are doing." Such a warning would be even more appropriate here.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 773097 [details] [diff] [review]
Add a memory reporter for ICU.

Review of attachment 773097 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine enough, but the atomic change necessitates enough change overall that I probably should look a second time.

This needs to be documented on MDN at <https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_SetICUMemoryFunctions>, and the function should be added to https://developer.mozilla.org/en-US/docs/SpiderMonkey/31 as new.  Copy one of the existing JSAPI reference pages, https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_GetClass or so, to get the style right.  Poke me when you're done with it, and I'll give it a second set of eyes for correctness/typos/etc.

::: js/src/jsapi.cpp
@@ +64,5 @@
>  #include "ion/PcScriptCache.h"
>  #include "js/CharacterEncoding.h"
> +#if ENABLE_INTL_API
> +#include "unicode/utypes.h"
> +#include "unicode/uclean.h"

Keep these alphabetical.

@@ +1147,5 @@
>      js_free(rt->defaultLocale);
>      js_delete(rt);
>  }
>  
> +#if ENABLE_INTL_API

Put this #if inside the method, and have it just |return true;| if !ENABLE_INTL_API.

@@ +1154,5 @@
> +                         JS_ICUFreeFn freeFn)
> +{
> +    UErrorCode status = U_ZERO_ERROR;
> +    u_setMemoryFunctions(context, allocFn, reallocFn, freeFn, &status);
> +    return status == U_ZERO_ERROR;

Yeah, U_SUCCESS.

::: js/src/jsapi.h
@@ +1760,5 @@
>  JS_DestroyRuntime(JSRuntime *rt);
>  
> +typedef void *(*JS_ICUAllocFn)(const void *context, size_t size);
> +typedef void *(*JS_ICUReallocFn)(const void *context, void *p, size_t size);
> +typedef void (*JS_ICUFreeFn)(const void *context, void *p);

Regarding typedef equivalency, put a

using mozilla::IsSame;
MOZ_STATIC_ASSERT((IsSame<JS_ICUAllocFn, UMemAllocFn),
                  "alloc function typedef mismatch");

and similar for all the others in JS_SetICUMemoryFunctions in jsapi.cpp, to guarantee this.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2625,5 @@
> +// Ideally it would be |size_t|, but JS_ATOMIC_* only works on |int32_t|.
> +// If ICU ever uses more than 2 GiB of memory, we'll have bigger problems than
> +// this counter being inaccurate.
> +//
> +static int32_t sSizeOfICU = 0;

Don't use jslock.h and all that.  Use mozilla/Atomic.h and mozilla::Atomic<size_t> instead.

Comment 12

[Nicholas Nethercote [inactive]]

> Regarding typedef equivalency, put a
> 
> using mozilla::IsSame;
> MOZ_STATIC_ASSERT((IsSame<JS_ICUAllocFn, UMemAllocFn),
>                   "alloc function typedef mismatch");
> 
> and similar for all the others in JS_SetICUMemoryFunctions in jsapi.cpp, to
> guarantee this.

I can't get this to work -- the static assertions fail.  It might be because the real definition in ICU is this:

  typedef void *U_CALLCONV UMemAllocFn(const void *context, size_t size);

where U_CALLCONV expands to __cdecl on MSVC?  Not sure.  I've omitted it for now.  If the typedefs didn't match we should get a compile error anyway.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: Add a memory reporter for ICU.

Comment 14

[Jeff Walden [:Waldo]]

Compile error seems reasonable enough to me.

Hmm, though.  Do we actually need APIs for this?  Or should all this memory simply be tracked by the JS engine, by default, underneath its own js/ memory hierarchy?  It's not clear to me, on second thought, that embedders should be given this kind of control.  If the engine can track this memory underneath itself, that seems preferable to adding APIs whose meaning/intended purpose is as vague as it is here.

Comment 15

[Nicholas Nethercote [inactive]]

The issue is the NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_ALLOC_FUN stuff -- that's code that ties in with DMD and lets DMD know that the memory has been reported.  It's important for our memory reporting verification, and it is code defined in xpcom/base/nsIMemoryReporterManager.idl and so can't be used directly from SpiderMonkey.

This patch has a single API function, which is the smallest interface I can think of to do this, while dealing with the constraint that most of our memory reporting machinery is outside of (and not visible to) SpiderMonkey.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 779036 [details] [diff] [review]
Add a memory reporter for ICU.

Review of attachment 779036 [details] [diff] [review]:
-----------------------------------------------------------------

Bleh.  This memory reporting machinery really should be in mfbt, and the mozalloc stuff should be there as well.  But that's clearly a tangent here, so I guess this approach works for now.

::: js/src/jsapi.h
@@ +1783,5 @@
> +typedef void (*JS_ICUFreeFn)(const void *context, void *p);
> +
> +// This function can be used to track memory used by ICU.
> +// Do not use it unless you know what you are doing!
> +extern JS_PUBLIC_API(bool)

Add something like "Don't use the context parameter passed to these functions!" to cover that base (see below).

@@ +1785,5 @@
> +// This function can be used to track memory used by ICU.
> +// Do not use it unless you know what you are doing!
> +extern JS_PUBLIC_API(bool)
> +JS_SetICUMemoryFunctions(const void *context, JS_ICUAllocFn allocFn, JS_ICUReallocFn reallocFn,
> +                         JS_ICUFreeFn freeFn);

Don't include the context argument here -- if we're not going to use it, and if we should move the memory-reporting stuff into something mfbt can use/provide, then the context isn't something people should depend on.  They don't get one for js_malloc/js_free and friends.  We shouldn't give them one here, particularly if (I hope) we can get rid of this method once JS has access to the mozalloc infrastructure.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +31,5 @@
>  #include "nsCycleCollectionNoteRootCallback.h"
>  #include "nsCycleCollectorUtils.h"
>  #include "nsScriptLoader.h"
>  #include "jsfriendapi.h"
> +#include "jslock.h"

Don't add this, no longer necessary.  We're trying to remove that header, actually, so extra includes of it are especially undesirable.

@@ +2484,5 @@
> +
> +static void*
> +ICUAlloc(const void* context, size_t size)
> +{
> +    void *p = js_malloc(size);

I think you want * on left and all for Gecko code, in all the methods here.  (You're not consistent inside methods here.)

@@ +2492,5 @@
> +
> +static void*
> +ICURealloc(const void* context, void* p, size_t size)
> +{
> +    sSizeOfICU -= ICUMallocSizeOfOnFree(p);

I would have

  size_t delta = 0 - ICUMallocSizeOfOnFree(p);
  void* pnew = realloc(p, size);
  delta += pnew ? ICUMallocSizeOfOnAlloc(pnew) : ICUMallocSizeOfOnAlloc(p);
  sSizeOfICU += delta;
  return pnew;

if it were me (to avoid a needless extra atomic op), but this works as well.

@@ +2493,5 @@
> +static void*
> +ICURealloc(const void* context, void* p, size_t size)
> +{
> +    sSizeOfICU -= ICUMallocSizeOfOnFree(p);
> +    void *pnew = realloc(p, size);

Should you be matching js_malloc with realloc and free?  Don't they both want to malloc/realloc/free or js_malloc/js_realloc/js_free?  Or maybe even moz_xmalloc/moz_xrealloc/moz_xfree or something?

@@ +2618,5 @@
>      // handlers.
>      JS_SetSourceHook(runtime, SourceHook);
>  
> +    if (!JS_SetICUMemoryFunctions(/* context = */ NULL, ICUAlloc, ICURealloc, ICUFree))
> +        NS_RUNTIMEABORT("JS_SetICUMemoryFunctions failed.");

We're kind of getting lucky this works.  u_setMemoryFunctions only works with a clean heap -- no ICU allocations yet.  It so happens that this runtime is the first one created, so ICU hasn't been initialized (implicitly, at the moment, but bug 890127 will change that), and this works.  But really it belongs in JS_Init, or (second best) slightly after the call to it.

Your patch and mine conflict badly in the patch-rejects sense.  If my patch for bug 896124 lands first, you should move this call into xpcom/build/nsXPComInit.cpp right after the JS_Init call there.  If yours lands first, I guess I'll do that move myself.

Comment 17

[Nicholas Nethercote [inactive]]

> Bleh.  This memory reporting machinery really should be in mfbt, and the
> mozalloc stuff should be there as well.  But that's clearly a tangent here,
> so I guess this approach works for now.

Memory reporters end up being registered with nsMemoryReporterManager, which is an XPCOM service.  So a big chunk of the memory reporting stuff will inevitably remain Gecko-only.

However, the NS_MEMORY_REPORTER_MALLOC_SIZEOF_ON_ALLOC_FUN stuff is independent of nsMemoryReporterManager.  So I tried moving it into MFBT, but it doesn't work because that macro relies on memory/replace/dmd/DMD.h which cannot be #included from MFBT :(  So AFAICT I have to stick with the current design.


> I think you want * on left and all for Gecko code, in all the methods here. 
> (You're not consistent inside methods here.)

Ah, but xpconnect is a style-free wonderland... having said that, mixing styles in a single function is bad, and I've fixed that up.


> Should you be matching js_malloc with realloc and free?

Yes!

> Or maybe even moz_xmalloc/moz_xrealloc/moz_xfree or something?

malloc/realloc/free will end up calling those functions.


> Your patch and mine conflict badly in the patch-rejects sense.  If my patch
> for bug 896124 lands first, you should move this call into
> xpcom/build/nsXPComInit.cpp right after the JS_Init call there.  If yours
> lands first, I guess I'll do that move myself.

Yep, whoever lands second can adjust.

Comment 18

[Nicholas Nethercote [inactive]]

> > Should you be matching js_malloc with realloc and free?
> 
> Yes!

And by "yes", I mean "no".

Comment 19

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c71b8f7089c0

> > Your patch and mine conflict badly in the patch-rejects sense.  If my patch
> > for bug 896124 lands first, you should move this call into
> > xpcom/build/nsXPComInit.cpp right after the JS_Init call there.  If yours
> > lands first, I guess I'll do that move myself.
> 
> Yep, whoever lands second can adjust.

Since I just landed, I guess you'll do the adjusting.  Thanks!

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c71b8f7089c0

Comment 21

[Jeff Walden [:Waldo]]

This still needs to be documented on these pages:

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_SetICUMemoryFunctions
https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JS_Init
https://developer.mozilla.org/en-US/docs/SpiderMonkey/31

Please make sure to note that this function, if called, must be called *before* JS_Init is called.  Feel free to point me at diffs of the changes you make if you want them reviewed.

Comment 22

[Nicholas Nethercote [inactive]]

Thanks for the reminder about the docs.

> https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/
> JS_SetICUMemoryFunctions

I've created this page.

> https://developer.mozilla.org/en-US/docs/SpiderMonkey/31

Here's the diff for that page:
https://developer.mozilla.org/en-US/docs/SpiderMonkey/31$compare?to=449741&from=448747