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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Const-ify k{Unix,Win}Charsets and kLangGroups

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Const-ify kCoefficientRgbY

This allows it to be shared between processes.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Const-ify dom encodings and similar arrays

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Const-ify mozilla::dom::ErrorFormatString

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Const-ify and shrink sack_array

This reduces its size from 5 KiB to 3 KiB, and makes it shareable between
processes.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Const-ify sWAIRoleMaps

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Const-ify keysymtab

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Const-ify many WebRTC arrays

As usual, I don't know if this code is from upstream and so whether changes
like this are ok.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Const-ify kKeyNames and kCodeNames

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Const-ify and shrink kEntities and kAttrEntities

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Const-ify ns{Default,Extra}MimeTypeEntry arrays

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Const-ify various sctp arrays and shrink sctp_hs_raise_drop

This makes some of them shareable between processes.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: Heap-allocate _progname

This saves almost 1 KiB per process.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: Const-ify sExtensionNames

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: Const-ify attrs

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Const-ify kPinset_* arrays

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Const-ify many kPacketMask* arrays

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: Const-ify GetCommandStr.kCommands

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: Const-ify SVGFEBlendElement::sModeMap and related arrays

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8729309 [details] [diff] [review]
Const-ify ns{Default,Extra}MimeTypeEntry arrays

r=me

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: Const-ify named CIDs

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: Const-ify libyuv vecs

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8729316 [details] [diff] [review]
Const-ify attrs

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

external library, redirect to owner

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

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/

Comment 27

[:Cykesiopka]

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.

Comment 28

[Eric Rescorla (:ekr)]

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.

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 8729304 [details] [diff] [review]
Const-ify keysymtab

This should also have internal linkage, if you are up for adding "static".

Comment 30

[Karl Tomlinson (:karlt)]

Comment on attachment 8729306 [details] [diff] [review]
Const-ify kKeyNames and kCodeNames

I wonder how much relocations like this cost startup.

Comment 31

[Karl Tomlinson (:karlt)]

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.

Comment 32

[u408661]

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

Comment 33

[u408661]

Comment on attachment 8729302 [details] [diff] [review]
Const-ify and shrink sack_array

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

Ditto...

Comment 34

[Nathan Froyd [:froydnj]]

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.

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8729302 [details] [diff] [review]
Const-ify and shrink sack_array

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

ditto

Comment 37

[Michael Tüxen]

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...

Comment 38

[Michael Tüxen]

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.

Comment 39

[Nicholas Nethercote [inactive]]

> >  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?

Comment 40

[Nathan Froyd [:froydnj]]

(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?

Comment 41

[Andrew McCreight [:mccr8]]

(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 42

[Michael Tüxen]

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.

Comment 43

[Nathan Froyd [:froydnj]]

(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!

Comment 44

[Nicholas Nethercote [inactive]]

(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.

Comment 45

[Nicholas Nethercote [inactive]]

Attachment: Const-ify and shrink octet_weight

Please forward review appropriately.

Comment 46

[Michael Tüxen]

(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...

Comment 47

[u408661]

Comment on attachment 8730361 [details] [diff] [review]
Const-ify and shrink octet_weight

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

Forwarding to :jesup again

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 49

[Nicholas Nethercote [inactive]]

Attachment: Const-ify kCoefficientRgbY

More changes required to build on Windows...

Comment 50

[Nicholas Nethercote [inactive]]

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.

Comment 51

[Nicholas Nethercote [inactive]]

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.

Comment 52

[Nicholas Nethercote [inactive]]

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.

Comment 53

[Nicholas Nethercote [inactive]]

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.

Comment 54

[Nicholas Nethercote [inactive]]

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.

Comment 55

[Nicholas Nethercote [inactive]]

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.

Comment 56

[Nicholas Nethercote [inactive]]

> 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 :(

Comment 57

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b540b97736ab5e83ac59cdadeeaa52e766522cd5
Bug 1255655 - Const-ify and shrink octet_weight. r=hurley.

Comment 58

[Nicholas Nethercote [inactive]]

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.

Comment 59

[Nicholas Nethercote [inactive]]

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.

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87dc4355a517
https://hg.mozilla.org/mozilla-central/rev/318273f25e6d
https://hg.mozilla.org/mozilla-central/rev/82c47146e4ac
https://hg.mozilla.org/mozilla-central/rev/ef70e996d4a3
https://hg.mozilla.org/mozilla-central/rev/a479bea8e02f
https://hg.mozilla.org/mozilla-central/rev/1b327a997498
https://hg.mozilla.org/mozilla-central/rev/8e9302df5f88
https://hg.mozilla.org/mozilla-central/rev/bd37c35f35c1
https://hg.mozilla.org/mozilla-central/rev/019b4826ceeb
https://hg.mozilla.org/mozilla-central/rev/3ca60a4a896a
https://hg.mozilla.org/mozilla-central/rev/c69cda4d6d6e
https://hg.mozilla.org/mozilla-central/rev/482cfa0f9567
https://hg.mozilla.org/mozilla-central/rev/07c1eeffe590
https://hg.mozilla.org/mozilla-central/rev/f94d743ff740
https://hg.mozilla.org/mozilla-central/rev/b540b97736ab

Comment 61

[Michael Tüxen]

SCTP related changes are committed upstream:
https://github.com/sctplab/usrsctp/commit/81055221ea20f04937df24ce078338041d465499

Comment 62

[Nicholas Nethercote [inactive]]

(In reply to Michael Tüxen from comment #61)
> SCTP related changes are committed upstream:
> https://github.com/sctplab/usrsctp/commit/
> 81055221ea20f04937df24ce078338041d465499

Thank you.

Comment 63

[Nils Ohlmeier [:drno]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=579f6a1c77806fc00bcf6181583e6b6aa6066f4d