Closed Bug 1255655 Opened 4 years ago Closed 4 years ago

Const-ify lots of variables to make them read-only

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 1 open bug)

Details

Attachments

(15 files, 8 obsolete files)

4.06 KB, patch
smontagu
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.09 KB, patch
baku
: review+
njn
: checkin+
Details | Diff | Splinter Review
993 bytes, patch
baku
: review+
njn
: checkin+
Details | Diff | Splinter Review
10.46 KB, patch
tbsaunde
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.21 KB, patch
karlt
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.37 KB, patch
karlt
: review+
njn
: checkin+
Details | Diff | Splinter Review
11.75 KB, patch
baku
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.51 KB, patch
bzbarsky
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.44 KB, patch
froydnj
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.73 KB, patch
mattwoodrow
: review+
njn
: checkin+
Details | Diff | Splinter Review
7.95 KB, patch
Cykesiopka
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.04 KB, patch
karlt
: review+
njn
: checkin+
Details | Diff | Splinter Review
889 bytes, patch
froydnj
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.49 KB, patch
jesup
: review+
njn
: checkin+
Details | Diff | Splinter Review
12.52 KB, patch
jrmuizel
: review+
njn
: checkin+
Details | Diff | Splinter Review
Lots of variables don't get marked with |const| when they could, which means they end up in the .data section unnecessarily.

By adding |const|, we can move them:

- to the .rodata section, if they don't contain pointers, which lets them be shared between processes.

- to the .data.rel.ro{,.local} section, otherwise, which doesn't allow sharing (alas) but might let the compiler optimize things a little.

Insufficient |const| is common in tables containing pointers. E.g. if you have an array of integers it's easy to get that right:

> static const int array_of_int[];

but for pointers people usually do this:

> static const T* array_of_T[];

Reading from right-to-left, each array entry is a "pointer to a T that is const".
What we want is this:

> static const T* const array_of_T[];

Which means "const pointer to a T that is const", i.e. the array entry itself is const.

I have a bunch of patches forthcoming. In a few of them I will also adjust the data in order to make it smaller.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attached patch Const-ify kCoefficientRgbY (obsolete) — Splinter Review
This allows it to be shared between processes.
Attachment #8729296 - Flags: review?(jmuizelaar)
Attachment #8729298 - Flags: review?(amarchesini)
Attachment #8729299 - Flags: review?(amarchesini)
Attached patch Const-ify and shrink sack_array (obsolete) — Splinter Review
This reduces its size from 5 KiB to 3 KiB, and makes it shareable between
processes.
Attachment #8729302 - Flags: review?(hurley)
Attachment #8729303 - Flags: review?(tbsaunde+mozbugs)
Attachment #8729304 - Flags: review?(karlt)
Attached patch Const-ify many WebRTC arrays (obsolete) — Splinter Review
As usual, I don't know if this code is from upstream and so whether changes
like this are ok.
Attachment #8729305 - Flags: review?(rjesup)
Attachment #8729306 - Flags: review?(karlt)
Attachment #8729296 - Flags: review?(jmuizelaar) → review+
This makes some of them shareable between processes.
Attachment #8729311 - Flags: review?(hurley)
This saves almost 1 KiB per process.
Attachment #8729314 - Flags: review?(nfroyd)
Attachment #8729315 - Flags: review?(matt.woodrow)
Attached patch Const-ify attrs (obsolete) — Splinter Review
Attachment #8729316 - Flags: review?(rjesup)
Attachment #8729317 - Flags: review?(cykesiopka.bmo)
Attachment #8729318 - Flags: review?(rjesup)
Attachment #8729319 - Flags: review?(karlt)
Comment on attachment 8729309 [details] [diff] [review]
Const-ify ns{Default,Extra}MimeTypeEntry arrays

r=me
Attachment #8729309 - Flags: review?(bzbarsky) → review+
Attachment #8729320 - Flags: review?(jwatt) → review+
Attachment #8729328 - Flags: review?(nfroyd)
Attached patch Const-ify libyuv vecs (obsolete) — Splinter Review
Attachment #8729336 - Flags: review?(rjesup)
Attachment #8729315 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8729305 [details] [diff] [review]
Const-ify many WebRTC arrays

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

a) split isac/* from others into a separate patch
b) This will require us to merge all these little changes on every update of webrtc.org... Can you either instead-of or in-addition-to file a bug and a patch (patches) for review in the upstream module at webrtc.org?  https://bugs.chromium.org/p/webrtc/issues/list and https://chromium.googlesource.com/external/webrtc
Attachment #8729305 - Flags: review?(rjesup) → review+
Comment on attachment 8729316 [details] [diff] [review]
Const-ify attrs

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

external library, redirect to owner
Attachment #8729316 - Flags: review?(rjesup) → review?(ekr)
Comment on attachment 8729318 [details] [diff] [review]
Const-ify many kPacketMask* arrays

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

Ditto on upstream webrtc.org comments
Attachment #8729318 - Flags: review?(rjesup) → review+
Comment on attachment 8729336 [details] [diff] [review]
Const-ify libyuv vecs

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

Ditto - upstream is at https://chromium.googlesource.com/libyuv/libyuv/
Attachment #8729336 - Flags: review?(rjesup) → review+
Attachment #8729298 - Flags: review?(amarchesini) → review+
Attachment #8729299 - Flags: review?(amarchesini) → review+
Attachment #8729307 - Flags: review?(amarchesini) → review+
Comment on attachment 8729317 [details] [diff] [review]
Const-ify kPinset_* arrays

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

The changes here look good, but there are two minor issues:
 - The header is regenerated each week by a script, so you'll need to update the script as well.
   - This is the line you want: https://hg.mozilla.org/mozilla-central/file/b8eeb6a19324/security/manager/tools/genHPKPStaticPins.js#l468
 - The patch didn't apply cleanly for me on close to tip of m-i.

Nothing major, so r+ assuming you fix the issues above and regenerate the header using the script.
Attachment #8729317 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8729316 [details] [diff] [review]
Const-ify attrs

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

I'm not saying that this is a bad patch, but we haven't really figured out our policy for engaging with nICEr, and this seems like a gratuitous change.

NJN: if you want to make changes here, you, byron, and I should have a call to discuss.
Attachment #8729316 - Flags: review?(ekr)
Comment on attachment 8729304 [details] [diff] [review]
Const-ify keysymtab

This should also have internal linkage, if you are up for adding "static".
Attachment #8729304 - Flags: review?(karlt) → review+
Comment on attachment 8729306 [details] [diff] [review]
Const-ify kKeyNames and kCodeNames

I wonder how much relocations like this cost startup.
Attachment #8729306 - Flags: review?(karlt) → review+
Comment on attachment 8729319 [details] [diff] [review]
Const-ify GetCommandStr.kCommands

This is the approach to avoid relocations, but I doubt this page gets marked read-only.  How do best to initialize pointers is a separate issue anyway.
Thanks for adding the const, even if it just makes the code clearer.
Attachment #8729319 - Flags: review?(karlt) → review+
Comment on attachment 8729311 [details] [diff] [review]
Const-ify various sctp arrays and shrink sctp_hs_raise_drop

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

Redirecting to rjesup, since he knows sctp better than I
Attachment #8729311 - Flags: review?(hurley) → review?(rjesup)
Comment on attachment 8729302 [details] [diff] [review]
Const-ify and shrink sack_array

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

Ditto...
Attachment #8729302 - Flags: review?(hurley) → review?(rjesup)
Attachment #8729328 - Flags: review?(nfroyd) → review+
Comment on attachment 8729314 [details] [diff] [review]
Heap-allocate _progname

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

r=me in principle, but see below.

Bonus points if you want to s/_progname/gProgname/.

::: toolkit/xre/nsSigHandlers.cpp
@@ +228,5 @@
>  #endif
>  
>  void InstallSignalHandlers(const char *ProgramName)
>  {
> +  const char* tmp = PL_strdup(ProgramName);

This leaks, right?  So we can't commit this patch as-is, unless I'm missing something.
Attachment #8729314 - Flags: review?(nfroyd) → review+
Comment on attachment 8729311 [details] [diff] [review]
Const-ify various sctp arrays and shrink sctp_hs_raise_drop

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

Strongly prefer to fix this upstream (imported library) than carry 'const' patches locally.  https://github.com/sctplab/
If Michael plans to take the patch, we could import locally; it will cause minor pain when we update next.
Attachment #8729311 - Flags: review?(rjesup)
Attachment #8729311 - Flags: review-
Attachment #8729311 - Flags: feedback?(tuexen)
Comment on attachment 8729302 [details] [diff] [review]
Const-ify and shrink sack_array

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

ditto
Attachment #8729302 - Flags: review?(rjesup)
Attachment #8729302 - Flags: review+
Attachment #8729302 - Flags: feedback?(tuexen)
Randell: Let me get most of it upstream. I need a couple of days for that. Please note that some changes will break SCTP. So don't include them as is...
Attachment #8729303 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8729302 [details] [diff] [review]
Const-ify and shrink sack_array

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

The changes to sctp_output.c are fine. I'll take them upstream.
The changes to sctp_header.h are NOT file. Please don't use them. They will break interoperability.

::: netwerk/sctp/src/netinet/sctp_header.h
@@ -272,5 @@
>  
>  /* Selective Ack (SACK) */
>  struct sctp_gap_ack_block {
> -	uint16_t start;		/* Gap Ack block start */
> -	uint16_t end;		/* Gap Ack block end */

This format is specified in
https://tools.ietf.org/html/rfc4960#section-3.3.4
Therefore changing the numbers from 16 bits to 8 bits will make the SCTP implementation non-compliant and will break interoperability.
I will not take this up stream.
Attachment #8729302 - Flags: feedback?(tuexen) → feedback-
> >  void InstallSignalHandlers(const char *ProgramName)
> >  {
> > +  const char* tmp = PL_strdup(ProgramName);
> 
> This leaks, right?  So we can't commit this patch as-is, unless I'm missing
> something.

It "leaks" in the sense that it's never freed. But it gets assigned to _progname which is a global variable, which is appropriate for this, no?
(In reply to Nicholas Nethercote [:njn] from comment #39)
> > >  void InstallSignalHandlers(const char *ProgramName)
> > >  {
> > > +  const char* tmp = PL_strdup(ProgramName);
> > 
> > This leaks, right?  So we can't commit this patch as-is, unless I'm missing
> > something.
> 
> It "leaks" in the sense that it's never freed. But it gets assigned to
> _progname which is a global variable, which is appropriate for this, no?

Sure, but LSan will complain about this, won't it?
(In reply to Nathan Froyd [:froydnj] from comment #40)
> Sure, but LSan will complain about this, won't it?

LSan shouldn't complain if the leaked object is in a global variable.
Comment on attachment 8729311 [details] [diff] [review]
Const-ify various sctp arrays and shrink sctp_hs_raise_drop

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

I'll integrate that upstream with the modifications in sctp_cc_functions.c

::: netwerk/sctp/src/netinet/sctp_cc_functions.c
@@ +1721,5 @@
>  				break;
>  			}
>  		}
>  		net->last_hs_used = indx;
> +		incr = (((int)sctp_cwnd_adjust[indx].increase) << 10);

I'll just use a (int32_t) here... And I'll add a cast when accessing drop_percent later.
Attachment #8729311 - Flags: feedback?(tuexen) → feedback-
(In reply to Andrew McCreight [:mccr8] from comment #41)
> (In reply to Nathan Froyd [:froydnj] from comment #40)
> > Sure, but LSan will complain about this, won't it?
> 
> LSan shouldn't complain if the leaked object is in a global variable.

Ah, I am enlightened.  Carry on, then!
Attachment #8729295 - Flags: review?(smontagu) → review+
(In reply to Michael Tüxen from comment #38)
> 
> The changes to sctp_output.c are fine. I'll take them upstream.

(In reply to Michael Tüxen from comment #42)
> 
> I'll integrate that upstream with the modifications in sctp_cc_functions.c
>
> [...]
> 
> I'll just use a (int32_t) here... And I'll add a cast when accessing
> drop_percent later.

Just to clarify: can I just forget about these ones and you will land them for me upstream? Thank you.
Flags: needinfo?(tuexen)
Please forward review appropriately.
Attachment #8730361 - Flags: review?(hurley)
(In reply to Nicholas Nethercote [:njn] from comment #44)
> (In reply to Michael Tüxen from comment #38)
> > 
> > The changes to sctp_output.c are fine. I'll take them upstream.
> 
> (In reply to Michael Tüxen from comment #42)
> > 
> > I'll integrate that upstream with the modifications in sctp_cc_functions.c
> >
> > [...]
> > 
> > I'll just use a (int32_t) here... And I'll add a cast when accessing
> > drop_percent later.
> 
> Just to clarify: can I just forget about these ones and you will land them
> for me upstream? Thank you.

Yepp. I'll merge it later this week. I'm currently on vacation...
Flags: needinfo?(tuexen)
Comment on attachment 8730361 [details] [diff] [review]
Const-ify and shrink octet_weight

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

Forwarding to :jesup again
Attachment #8730361 - Flags: review?(hurley) → review?(rjesup)
Comment on attachment 8730361 [details] [diff] [review]
Const-ify and shrink octet_weight

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

Add a comment that this should be retained until we land an upstream version with this
Attachment #8730361 - Flags: review?(rjesup) → review+
More changes required to build on Windows...
Attachment #8730473 - Flags: review?(jmuizelaar)
Attachment #8729296 - Attachment is obsolete: true
Attachment #8730473 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/87dc4355a517fdd4b4273ed0043195a3c2610efd
Bug 1255655 - Const-ify k{Unix,Win}Charsets and kLangGroups. r=smontagu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/318273f25e6d460d8e267353efe4f72406587ca1
Bug 1255655 - Const-ify kCoefficientRgbY. r=jrmuizel.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c47146e4ac3c7cf312db01d2b63f10f5bf09a6
Bug 1255655 - Const-ify sWAIRoleMaps. r=tbsaunde.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef70e996d4a3e74ce1ca8503f99b7f87253f95f4
Bug 1255655 - Const-ify dom encodings and similar arrays. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a479bea8e02f02da9215be9535e05143aadc432b
Bug 1255655 - Const-ify mozilla::dom::ErrorFormatString. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b327a9974987a532deeccf0cc6603d3a22a4add
Bug 1255655 - Const-ify keysymtab. r=karlt.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e9302df5f88f7193a25dc35f3f45ff2007b6337
Bug 1255655 - Const-ify kKeyNames and kCodeNames. r=karlt.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd37c35f35c1cd50fcf2f30e4a0397cd97cb9554
Bug 1255655 - Const-ify and shrink kEntities and kAttrEntities. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/019b4826ceebead0971afe661ab97234168fc15d
Bug 1255655 - Const-ify ns{Default,Extra}MimeTypeEntry arrays. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca60a4a896a8b8e933ca736dbbc09bb0392222a
Bug 1255655 - Heap-allocate _progname. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c69cda4d6d6ee1bda9bbd5ff952a3369b5e6fc43
Bug 1255655 - Const-ify sExtensionNames. r=mattwoodrow.

https://hg.mozilla.org/integration/mozilla-inbound/rev/482cfa0f9567d2a383a02da3e5256fb963b1b8fd
Bug 1255655 - Const-ify kPinset_* arrays. r=cykesiopka.

https://hg.mozilla.org/integration/mozilla-inbound/rev/07c1eeffe5902eb9e8612c15a0ed5afee3d75245
Bug 1255655 - Const-ify GetCommandStr.kCommands. r=karlt.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f94d743ff7406677f3be229e8406661d073b2ec7
Bug 1255655 - Const-ify named CIDs. r=froydnj.
Attachment #8729295 - Flags: checkin+
Attachment #8729298 - Flags: checkin+
Attachment #8729299 - Flags: checkin+
Attachment #8729303 - Flags: checkin+
Attachment #8729304 - Flags: checkin+
Attachment #8729306 - Flags: checkin+
Attachment #8729307 - Flags: checkin+
Attachment #8729314 - Flags: checkin+
Attachment #8729315 - Flags: checkin+
Attachment #8729317 - Flags: checkin+
Attachment #8729319 - Flags: checkin+
Attachment #8729328 - Flags: checkin+
Attachment #8729309 - Flags: checkin+
Comment on attachment 8729302 [details] [diff] [review]
Const-ify and shrink sack_array

Marking this obsolete because Michael said he would take part of it upstream, and the other part cannot land.
Attachment #8729302 - Attachment is obsolete: true
Comment on attachment 8729311 [details] [diff] [review]
Const-ify various sctp arrays and shrink sctp_hs_raise_drop

Marking this obsolete because Michael said he would land a modified version upstream.
Attachment #8729311 - Attachment is obsolete: true
Comment on attachment 8729316 [details] [diff] [review]
Const-ify attrs

Marking this obsolete because it's not worth the effort to land upstream when it only moves the array into the .data.rel.ro.local segment.
Attachment #8729316 - Attachment is obsolete: true
Attachment #8730473 - Flags: checkin+
Comment on attachment 8729318 [details] [diff] [review]
Const-ify many kPacketMask* arrays

Marking this obsolete because it's not worth the effort to land upstream when it only moves the arrays into the .data.rel.ro.local segment.
Attachment #8729318 - Attachment is obsolete: true
Comment on attachment 8729305 [details] [diff] [review]
Const-ify many WebRTC arrays

Marking this obsolete because it's not worth the effort to land upstream when it only moves the arrays into the .data.rel.ro.local segment.
> Ditto - upstream is at https://chromium.googlesource.com/libyuv/libyuv/

I visited that link and all the documentation links give a 404 so I have no idea how to contribute an upstream patch :(
Attachment #8730361 - Flags: checkin+
Attachment #8729305 - Attachment is obsolete: true
Comment on attachment 8729320 [details] [diff] [review]
Const-ify SVGFEBlendElement::sModeMap and related arrays

Marking this obsolete because there are *lots* more similar cases in SVG but it's not worth the effort to fix them all when it only moves the arrays into the .data.rel.ro.local segment.
Attachment #8729320 - Attachment is obsolete: true
Comment on attachment 8729336 [details] [diff] [review]
Const-ify libyuv vecs

Obsoleting because I don't have the energy to work out how to upstream this.
Attachment #8729336 - Attachment is obsolete: true
(In reply to Michael Tüxen from comment #61)
> SCTP related changes are committed upstream:
> https://github.com/sctplab/usrsctp/commit/
> 81055221ea20f04937df24ce078338041d465499

Thank you.
You need to log in before you can comment on or make changes to this bug.