Miscellaneous SpiderMonkey cleanups

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
The following are a variety of cleanup patches.
(Assignee)

Comment 1

5 years ago
Created attachment 806846 [details] [diff] [review]
registerset-cleanup.patch

Fix a little duplicated code.
Attachment #806846 - Flags: review?(hv1989)
(Assignee)

Comment 2

5 years ago
Created attachment 806847 [details] [diff] [review]
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 :).
Attachment #806847 - Flags: review?(hv1989)
(Assignee)

Comment 3

5 years ago
Created attachment 806848 [details] [diff] [review]
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.
Attachment #806848 - Flags: review?(hv1989)
(Assignee)

Comment 4

5 years ago
Created attachment 806852 [details] [diff] [review]
js-consts.patch

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)

Updated

5 years ago
Attachment #806848 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 7

5 years ago
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+
(Assignee)

Comment 10

5 years ago
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.