Closed Bug 918023 Opened 6 years ago Closed 6 years ago

Miscellaneous SpiderMonkey cleanups

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: sunfish, Assigned: sunfish)

Details

Attachments

(4 files)

The following are a variety of cleanup patches.
Fix a little duplicated code.
Attachment #806846 - Flags: review?(hv1989)
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 :).
Attachment #806847 - Flags: review?(hv1989)
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.
Attachment #806848 - Flags: review?(hv1989)
Attached patch js-consts.patchSplinter Review
A pretty simple constification patch. No const_casts and relatively low impact outside of the main changes.
Attachment #806852 - Flags: review?(hv1989)
Attachment #806846 - Flags: review?(hv1989) → review+
Attachment #806847 - Flags: review?(hv1989) → review+
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.
Attachment #806848 - Flags: review?(hv1989) → review?(kvijayan)
Attachment #806848 - Flags: review?(kvijayan) → review+
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 :).
Attachment #806852 - Flags: review?(hv1989) → review?(n.nethercote)
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.
Attachment #806852 - Flags: review?(n.nethercote) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/b340b241e433
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.