503463 - TM: avoid integer / * for trigger factor in allocation path

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

Attachment: patch

Comment 1

[David Mandelin [:dmandelin]]

Comment on attachment 387818 [details] [diff] [review]
patch

>-    rt->gcTriggerFactor = (uint32) -1;
>+    js_SetGCTriggerFactor(rt, (uint32) -1);
> 
>     /*
>      * The assigned value prevents GC from running when GC memory is too low
>      * (during JS engine start).
>      */
>     rt->gcLastBytes = 8192;

It would seem that js_SetGCTriggerFactor must precede setting rt->gcLastBytes here. But see below for my preferred solution.

>+void
>+js_SetGCTriggerFactor(JSRuntime *rt, uint32 factor)
>+{
>+    JS_ASSERT(factor >= 100);
>+
>+    rt->gcTriggerFactor = factor;
>+    rt->gcTriggerBytes = rt->gcLastBytes / 100 * factor;
>+}

It's probably better to do

    rt->gcTriggerBytes = rt->gcLastBytes * factor / 100;

for rounding purposes. If overflow is a concern |factor/100| can be parenthesized.

>     rt->gcLastBytes = rt->gcBytes;
>+    js_SetGCTriggerFactor(rt, rt->gcTriggerFactor); // recalculate threshold

I think it would be cleaner to add an internal API:

    js_SetGCLastBytes(Runtime *rt, size_t lastBytes);

and have that update rt->gcTriggerBytes if necessary.

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #1)
> I think it would be cleaner to add an internal API:
> 
>     js_SetGCLastBytes(Runtime *rt, size_t lastBytes);
> 
> and have that update rt->gcTriggerBytes if necessary.

Feel free to make this JSRuntime::setGCLastBytes, no need for more C-style code in the C++ codebase.

/be

Comment 4

[Andreas Gal :gal]

Attachment: with C++-isms

Comment 5

[Andreas Gal :gal]

Attachment: correct patch

Comment 6

[David Mandelin [:dmandelin]]

Comment on attachment 388262 [details] [diff] [review]
correct patch

>+void
>+JSRuntime::SetGCTriggerFactor(uint32 factor)
>+{
>+    JS_ASSERT(factor >= 100);
>+
>+    gcTriggerFactor = factor;
>+}
>+
>+void
>+JSRuntime::SetGCLastBytes(size_t lastBytes)
>+{
>+    gcLastBytes = lastBytes;
>+    gcTriggerBytes = lastBytes * gcTriggerFactor / 100;
>+}
>+

I had expected SetGCTriggerFactor to update gcTriggerBytes as well, but I guess it's probably not important exactly when the trigger factor gets applied.

Comment 7

[Andreas Gal :gal]

I added SetGCLastBytes(gcLastBytes) to SetGCTriggerFactor as per comment #6

Comment 8

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/2073d5aae8b6

Comment 9

[Jeff Walden [:Waldo]]

Those should be JSRuntime::setGCLastBytes and JSRuntime::setGCTriggerFactor (note lowercase 's') to conform to the existing style of having member methods start with lowercase, continue with each word uppercased.

Comment 10

[Andreas Gal :gal]

I am not aware of that convention, but I am not opposed either.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> I am not aware of that convention, but I am not opposed either.

Look at your own code in jstracer.h! Method names are camelCaps.

rs=me on followup fix.

/be

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2073d5aae8b6

Comment 13

[Andreas Gal :gal]

This patch triggers a ton of gcs. Something bad bad. Please back out.

Comment 14

[Peter Van der Beken [:peterv]]

I backed this out from mozilla-central just to be sure (trying to undo all the orange from the latest TM merge).

Comment 15

[Boris Zbarsky [:bzbarsky]]

So... Patch has:

+    gcTriggerBytes = lastBytes * gcTriggerFactor / 100;

In the browser, gcTriggerFactor is 1600.  So if lastBytes is on the order of 2.7MB (or any multiple thereof), we get gcTriggerBytes very small, since that first multiplication overflows.  I think you really want those parens per comment 1.

Comment 16

[Andreas Gal :gal]

Thanks for backing out and sorry for the m-c bustage. I will do some testing with the proposed parens and if that works re-land.

Comment 17

[Andreas Gal :gal]

Attachment: avoid 32-bit overflow

Comment 18

[Andreas Gal :gal]

Confirmed to fix the crazy GCing.

Comment 19

[David Mandelin [:dmandelin]]

+    gcTriggerBytes = lastBytes * (gcTriggerFactor / 100);

This still overflows if gcTriggerFactor is -1, as it is on initialization. Apparently that yields a gcTriggerBytes of 4123168604, which would explain why the patch works well in initial tests. 

Andreas mentioned using 64-bit arithmetic to compute gcTriggerBytes. That sounds like a good idea. Also, we should consider defining -1 to be an 'infinity' value, that always set gcTriggerBytes to 2^32-1. But that still wouldn't fix the overflow problem in the presence of large heaps and/or large gcTriggerFactors.

Comment 20

[Boris Zbarsky [:bzbarsky]]

What's the right behavior in those cases?  If gcTriggerFactor/100 * lastBytes would overflow, would we want to just pin gcTriggerBytes to its max value?

If so, then you could just check for overflow explicitly:

  if (gcTriggerFactor == -1) {
    gcTriggerBytes = size_t_max;
  } else {
    size_t factor = gcTriggerFactor / 100;
    if (size_t_max / factor > lastBytes)
      gcTriggerBytes = size_t_max;
    else
      gcTriggerBytes = factor * lastBytes;
  }

or some such...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Might want >= lastBytes, given integer division.

Comment 22

[Andreas Gal :gal]

Attachment: really, really avoid 32-bit overflow

Comment 23

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/b5aaeea0f483

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b5aaeea0f483