1527225 - Use mozilla::Clamp in Nursery sizing calculations

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Paul Bone [:pbone]]

See

https://searchfox.org/mozilla-central/source/js/src/gc/Nursery.cpp#1177,1182

This could be made more readable with a JS_CLAMP macro.

Comment 1

[Jan de Mooij [:jandem]]

See mozilla::Clamp: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/mfbt/MathAlgorithms.h#494

(In general don't add new macros where inline functions suffice.)

Comment 2

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #1)

See mozilla::Clamp: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/mfbt/MathAlgorithms.h#494

(In general don't add new macros where inline functions suffice.)

Oh thanks. yeah, I realised that sfink (in a code review) was hinting at this also. But I don't think either of us realised that it was already defined as mozilla::Clamp.

Thanks.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1527225 - Use mozilla::Clamp in maybeResizeNursery() r=jandem!

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Jan de Mooij [:jandem] from comment #1)

See mozilla::Clamp: https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/mfbt/MathAlgorithms.h#494

Ah, thanks! Yeah, I looked for it next to Min()/Max() at https://searchfox.org/mozilla-central/source/mfbt/TemplateLib.h#31

Should that be merged into MathAlgorithms.h?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43baecd33ce6
Use mozilla::Clamp in maybeResizeNursery() r=jandem

Comment 6

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 43baecd33ce6 (Bug 1527225) for jsreftest crashes at [@ js::Nursery::maybeResizeNursery(JS::GCReason)] and assertion failures:

Backout: https://hg.mozilla.org/integration/autoland/rev/c6829642e2d081d1a1be21b828a5eb7947eedc52

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=43baecd33ce64f6c09114f6463da2ceffb03bebe&selectedJob=228282740

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228282740&repo=autoland&lineNumber=1412
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228283239&repo=autoland&lineNumber=1553

There are also some xpcshell failures too:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228282734&repo=autoland&lineNumber=1664

Comment 7

[Paul Bone [:pbone]]

Thanks cbrindusan,

Turns out when I tested this I had other patches with it and those other patches fixed these problems. My bad.

Comment 8

[Paul Bone [:pbone]]

Attachment: Bug 1527225 - Use mozilla::Clamp in maybeResizeNursery() r=jandem!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/632b98194776
Use mozilla::Clamp in maybeResizeNursery() r=sfink

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/632b98194776

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

Ah, thanks! Yeah, I looked for it next to Min()/Max() at https://searchfox.org/mozilla-central/source/mfbt/TemplateLib.h#31

Should that be merged into MathAlgorithms.h?

Yeah, those probably could stand to be moved over. "template lib" is a bit of a bad name, really, but it was the fast'n'easy choice back in the day. Mind spinning up a bug/patch for it?