918023 - Miscellaneous SpiderMonkey cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

The following are a variety of cleanup patches.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: registerset-cleanup.patch

Fix a little duplicated code.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: registerset-maybe-take.patch

While I'm here, rename maybeTake to takeUnchecked for consistency. I don't have a preference for which is better, I just want them to be the same :).

Comment 3

[Dan Gohman [:sunfish]]

Attachment: minor-cleanups.patch

This patch is just a collection of odds and ends. It adds a comment, fixes a magic number, and removes a copy+pasted comment which doesn't apply.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: js-consts.patch

A pretty simple constification patch. No const_casts and relatively low impact outside of the main changes.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 806848 [details] [diff] [review]
minor-cleanups.patch

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

I'm gonna forward this review to Kannan for the baselineRegisters-x64.h comment removal.

Comment 6

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8304a4e8413
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc4927b98536
https://hg.mozilla.org/integration/mozilla-inbound/rev/d39c487bd93f

Comment 7

[Dan Gohman [:sunfish]]

Comment on attachment 806852 [details] [diff] [review]
js-consts.patch

Not the most exciting patch in the world, but the good news is that this ought to be the last big const patch for the foreseeable future :).

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8304a4e8413
https://hg.mozilla.org/mozilla-central/rev/bc4927b98536
https://hg.mozilla.org/mozilla-central/rev/d39c487bd93f

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 806852 [details] [diff] [review]
js-consts.patch

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

How do you find all these?  Do you have a tool?

It all looks fine except for the conversion of all the *Family variables from int to char -- why did you do that?  I'm not certain it's wrong, but I can't see where these values are actually used, other than their addresses being passed around as void*.  Or is that the point... do we only ever look at their address to determine their identity?

If that's the case, r=me.  And it sure would be nice to have a comment explaining it.  And I guessed you changed the type to use less memory?  The saving is trivial enough I'd be inclined to leave it unchanged, myself.

Comment 10

[Dan Gohman [:sunfish]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Comment on attachment 806852 [details] [diff] [review]
> js-consts.patch
> 
> Review of attachment 806852 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How do you find all these?  Do you have a tool?

My main tool for these is readelf. Variables in the .data section (as opposed to .rodata, .data.rel.ro, or even .bss) are very likely to be candidates for const or constexpr. This patch along with the one in bug 920061 eliminate just about all of them in the JS engine code.

The one interesting variable not made const yet is number_constants in jsnum.cpp. Its data is actually constant, but there's no portable way to write an initializer for it :(.

> It all looks fine except for the conversion of all the *Family variables
> from int to char -- why did you do that?  I'm not certain it's wrong, but I
> can't see where these values are actually used, other than their addresses
> being passed around as void*.  Or is that the point... do we only ever look
> at their address to determine their identity?
> 
> If that's the case, r=me.  And it sure would be nice to have a comment
> explaining it.  And I guessed you changed the type to use less memory?  The
> saving is trivial enough I'd be inclined to leave it unchanged, myself.

Yes; these variables are only used for their address. The memory savings is negligible; char just seems to be a more idiomatic type for a variable whose only use is to have a unique address. I'll add comments about this.

Comment 11

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c3d1c9241d7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for a Windows debug TestEndian asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac09e1a3fa46

https://tbpl.mozilla.org/php/getParsedLog.php?id=28293933&tree=Mozilla-Inbound

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Nevermind that. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b340b241e433

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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