10 ms·

Add a const here, delete a const there

147 points by nikbackm 10y ago

cixin 10y ago
Wow awesome! It's partly these little optimizations that keeps C/C++ going. I've never considered that const might have a performance benefit (and mostly just use it as a compile time check). But will consider it in the future. Would be interesting to see performance benchmarks for the changes made here, as well as applying const in general.
- brucedawson 10y ago
  Ignoring the VC++ compiler bug/quirk, the benefits for adding const tend to be small and hard to measure. How do you measure the possible reduction in private pages? How do you measure the productivity gains from slightly fewer corrupted globals. Usually you don't. You just tag your const globals as such and then get on with your day. Once you develop the habit the cost is zero so measuring the benefits isn't necessary.
tech2 10y ago
I'm interested in the possible security improvements in the change which didn't seem to be mentioned at all. Is all this data already in an NX area, or was it somehow already protected via some other means?
- xyzzy123 10y ago
  Already nx but rw -> r with some code (rx) removed also. Not really security improvements from a w^x perspective.
- brucedawson 10y ago
  None of the pointers were code pointers so there are probably no security improvements. It might protect Chrome from a few hard to diagnose crashes because now if a stray write hits those arrays it crashes instantly instead of misbehaving later.
adrianN 10y ago
So will they put the consts back after the VS bug is fixed?
- rkangel 10y ago
  If all the of the things which have had 'const' removed are members of structs that are themselves 'const', then there is no need to re-add them.
  - deleted 10y ago
    [deleted]
deleted 10y ago
[deleted]
revelation 10y ago
For embedded systems that extra const can mean the difference between having that data in SRAM or Flash.
wfunction 10y ago
In my experience you should NEVER have const instance fields. Ever. If you want a const reference, just make a pointer to const. If you need a const value, just leave it mutable and access it through a const getter or work around it some other way. The language just doesn't seem to have been designed to make this particular feature actually worthwhile.
- keldaris 10y ago
  It's very worthwhile for performance, and somewhat helps limit basic programmer errors. I can't imagine a single reason to tell people not to use const instance fields. On the other hand, telling people not to treat const as an absolute guarantee is obviously good advice and what you said about const& and getters is spot on.
  - wfunction 10y ago
    You're gonna need a citation (preferably, an example) for the performance claim. As for this comment of yours: > I can't imagine a single reason to tell people not to use const instance fields. That's weird, did you not read the blog post? It literally gave you a reason not to, you don't need to imagine anything. If you don't think it's a compelling reason then that's another story, but it's a valid reason.
    - keldaris 10y ago
      It's impossible to provide general benchmarks, since the impact will be completely case specific. However, the reason I didn't provide a citation is because the OP already gives an example - blink::serializedCharacterData gets moved over the read-only data segment if you add const, with obvious consequences for, say, thread-local accesses in tight loops. In my own code, I've seen adding a const modifier result in 20-60% perf improvement in fairly extreme cases. In other cases (probably most cases), it won't change much. More importantly, I've never seen an example where adding const would ever decrease CPU performance by any amount at all (other than compiler bugs, which can ruin any language feature). Accordingly, my personal recommendation is that everything that can be const should be. It's an easy thing to do that may help and definitely won't hurt. > That's weird, did you not read the blog post? It literally gave you a reason not to, you don't need to imagine anything. If you don't think it's a compelling reason then that's another story, but it's a valid reason. That's an outright compiler bug, not a general language reason. Obviously, it's a valid caveat if you're using that specific compiler and care about that edge case, but it's hardly a general argument.
chamakits 10y ago
WARNING: I believe the site has some malware injected in it. At least the mobile version of it. After a few second it redirected me to some site with a fake Facebook skin.
- dzdt 10y ago
  I confirm this mal-redirect on mobile.
  - josteink 10y ago
    Firefox mobile unaffected.
- lorenzhs 10y ago
  Once again, this demonstrates that blocking advertisements is a really good idea from a security perspective. It doesn't just reduce clutter and speed up page loading times, but also prevents loads of malware issues. See also https://twitter.com/swiftonsecurity/status/628401554907725824 https://twitter.com/swiftonsecurity/status/62840155490772582...
  - iokanuon 10y ago
    AdAway's Android host file seems to block it just fine, you don't even need a fancy adblock plugin.
dzdt 10y ago
This is begging for a C++ FQA style hate blog post. Paging Yossi Kreinin? Seriously, "Delete const three times to move 53,064 bytes to read-only data segment and save 729,168 bytes of code." WTF? [1] http://yosefk.com/c++fqa/index.html http://yosefk.com/c++fqa/index.html [2] http://yosefk.com/blog/ http://yosefk.com/blog/
- lmm 10y ago
  It's MSVC only. They're going to fix it. And it doesn't affect the semantics, only performance. I mean sure, it's a bug, but it's not FQA territory.
- masklinn 10y ago
  It's a compiler issue, nothing to do with C++ so wouldn't be an FQA entry.
  - CalChris 10y ago
    I can see that the removing const case is a compiler issue. How is the adding const case a compiler issue? Shouldn't the result be the same on Clang and GCC?
    - sqeaky 10y ago
      That adding const open optimization opportunities for the compiler is not surprising to anyone who knows what const is for. Removing const to cause optimization is a big wtf and that is why it is worth debating.
      - brucedawson 10y ago
        Actually, with the exception of the modest benefits from moving globals to the read-only data segment it is rare to get optimization benefits from tagging something as const. const is for enforcing correctness by the compiler. It tells the compiler about intent, but it doesn't give the compiler any extra information that it can use for optimizing. Example: If you mark a member function as const then the compiler knows that you can't modify member variables. However the compiler won't use that to optimize the function - what it will use is its own data flow analysis that independently shows that you aren't modifying any member variables. The distinction becomes important if you call some non-member function whose definition is not visible. At that point the compiler has to assume that that function might modify the object, though a separate cached pointer. So, the fact that the member function is const becomes useless as soon as the the compiler can't analyze code flow anymore. I believe.
bla2 10y ago
MSVC is such a bad compiler. Yes, they are working on it, it's getting better, but this is apparently a fairly new regression.
- gcp 10y ago
  Working around the following bug caused Firefox's size to drop by over 200k. We should try fiddling with const, too :-) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78323 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78323
- brucedawson 10y ago
  Chrome continues to use MSVC on Windows because it generates better code than clang. So, MSVC is the worst compiler, except for the alternatives?
  - obl 10y ago
    Would appreciate a reference on that if you have it handy. Is this more of a whole program measured X% regression on some perf benchs, or specific target cases where the codegen is notably worse ?
    - brucedawson 10y ago
      crbug.com/457078 talks about binary size in clang-cl versus VC++. On x86 builds clang-cl produces ~14% larger binaries. On x64 builds it produces ~3.5% smaller binaries. My recent changes have made the comparison tougher for clang-cl by making the VC++ binaries smaller :-)
    - brucedawson 10y ago
      I should probably take back my claim because I can't find the evidence to back it up and I might actually be wrong. I thought I remembered that there were some important cases where VC++ generated better code for Chrome, and some binary size advantages, but I can't find my references.
  - Joky 10y ago
voltagex_ 10y ago
Offtopic sidenote: I wish more open source projects had instructions as good as https://www.chromium.org/developers/how-tos/debugging-on-windows https://www.chromium.org/developers/how-tos/debugging-on-win...

147 points by nikbackm 10y ago

cixin 10y ago

Wow awesome! It's partly these little optimizations that keeps C/C++ going. I've never considered that const might have a performance benefit (and mostly just use it as a compile time check). But will consider it in the future. Would be interesting to see performance benchmarks for the changes made here, as well as applying const in general.

brucedawson 10y ago
Ignoring the VC++ compiler bug/quirk, the benefits for adding const tend to be small and hard to measure. How do you measure the possible reduction in private pages? How do you measure the productivity gains from slightly fewer corrupted globals. Usually you don't. You just tag your const globals as such and then get on with your day. Once you develop the habit the cost is zero so measuring the benefits isn't necessary.

tech2 10y ago

I'm interested in the possible security improvements in the change which didn't seem to be mentioned at all. Is all this data already in an NX area, or was it somehow already protected via some other means?

xyzzy123 10y ago
Already nx but rw -> r with some code (rx) removed also. Not really security improvements from a w^x perspective.
brucedawson 10y ago
None of the pointers were code pointers so there are probably no security improvements. It might protect Chrome from a few hard to diagnose crashes because now if a stray write hits those arrays it crashes instantly instead of misbehaving later.

adrianN 10y ago

So will they put the consts back after the VS bug is fixed?

rkangel 10y ago
If all the of the things which have had 'const' removed are members of structs that are themselves 'const', then there is no need to re-add them.
- deleted 10y ago
  [deleted]

deleted 10y ago

[deleted]

revelation 10y ago

For embedded systems that extra const can mean the difference between having that data in SRAM or Flash.

wfunction 10y ago

In my experience you should NEVER have const instance fields. Ever. If you want a const reference, just make a pointer to const. If you need a const value, just leave it mutable and access it through a const getter or work around it some other way. The language just doesn't seem to have been designed to make this particular feature actually worthwhile.

keldaris 10y ago
It's very worthwhile for performance, and somewhat helps limit basic programmer errors. I can't imagine a single reason to tell people not to use const instance fields. On the other hand, telling people not to treat const as an absolute guarantee is obviously good advice and what you said about const& and getters is spot on.
- wfunction 10y ago
  You're gonna need a citation (preferably, an example) for the performance claim. As for this comment of yours: > I can't imagine a single reason to tell people not to use const instance fields. That's weird, did you not read the blog post? It literally gave you a reason not to, you don't need to imagine anything. If you don't think it's a compelling reason then that's another story, but it's a valid reason.
  - keldaris 10y ago
    It's impossible to provide general benchmarks, since the impact will be completely case specific. However, the reason I didn't provide a citation is because the OP already gives an example - blink::serializedCharacterData gets moved over the read-only data segment if you add const, with obvious consequences for, say, thread-local accesses in tight loops. In my own code, I've seen adding a const modifier result in 20-60% perf improvement in fairly extreme cases. In other cases (probably most cases), it won't change much. More importantly, I've never seen an example where adding const would ever decrease CPU performance by any amount at all (other than compiler bugs, which can ruin any language feature). Accordingly, my personal recommendation is that everything that can be const should be. It's an easy thing to do that may help and definitely won't hurt. > That's weird, did you not read the blog post? It literally gave you a reason not to, you don't need to imagine anything. If you don't think it's a compelling reason then that's another story, but it's a valid reason. That's an outright compiler bug, not a general language reason. Obviously, it's a valid caveat if you're using that specific compiler and care about that edge case, but it's hardly a general argument.

chamakits 10y ago

WARNING: I believe the site has some malware injected in it. At least the mobile version of it. After a few second it redirected me to some site with a fake Facebook skin.

dzdt 10y ago
I confirm this mal-redirect on mobile.
- josteink 10y ago
  Firefox mobile unaffected.
lorenzhs 10y ago
Once again, this demonstrates that blocking advertisements is a really good idea from a security perspective. It doesn't just reduce clutter and speed up page loading times, but also prevents loads of malware issues. See also https://twitter.com/swiftonsecurity/status/628401554907725824 https://twitter.com/swiftonsecurity/status/62840155490772582...
- iokanuon 10y ago
  AdAway's Android host file seems to block it just fine, you don't even need a fancy adblock plugin.

dzdt 10y ago

This is begging for a C++ FQA style hate blog post. Paging Yossi Kreinin? Seriously, "Delete const three times to move 53,064 bytes to read-only data segment and save 729,168 bytes of code." WTF? [1] http://yosefk.com/c++fqa/index.html http://yosefk.com/c++fqa/index.html [2] http://yosefk.com/blog/ http://yosefk.com/blog/

lmm 10y ago
It's MSVC only. They're going to fix it. And it doesn't affect the semantics, only performance. I mean sure, it's a bug, but it's not FQA territory.
masklinn 10y ago
It's a compiler issue, nothing to do with C++ so wouldn't be an FQA entry.
- CalChris 10y ago
  I can see that the removing const case is a compiler issue. How is the adding const case a compiler issue? Shouldn't the result be the same on Clang and GCC?
  - sqeaky 10y ago
    That adding const open optimization opportunities for the compiler is not surprising to anyone who knows what const is for. Removing const to cause optimization is a big wtf and that is why it is worth debating.
    - brucedawson 10y ago
      Actually, with the exception of the modest benefits from moving globals to the read-only data segment it is rare to get optimization benefits from tagging something as const. const is for enforcing correctness by the compiler. It tells the compiler about intent, but it doesn't give the compiler any extra information that it can use for optimizing. Example: If you mark a member function as const then the compiler knows that you can't modify member variables. However the compiler won't use that to optimize the function - what it will use is its own data flow analysis that independently shows that you aren't modifying any member variables. The distinction becomes important if you call some non-member function whose definition is not visible. At that point the compiler has to assume that that function might modify the object, though a separate cached pointer. So, the fact that the member function is const becomes useless as soon as the the compiler can't analyze code flow anymore. I believe.

bla2 10y ago

MSVC is such a bad compiler. Yes, they are working on it, it's getting better, but this is apparently a fairly new regression.

gcp 10y ago
Working around the following bug caused Firefox's size to drop by over 200k. We should try fiddling with const, too :-) https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78323 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78323
brucedawson 10y ago
Chrome continues to use MSVC on Windows because it generates better code than clang. So, MSVC is the worst compiler, except for the alternatives?
- obl 10y ago
  Would appreciate a reference on that if you have it handy. Is this more of a whole program measured X% regression on some perf benchs, or specific target cases where the codegen is notably worse ?
  - brucedawson 10y ago
    crbug.com/457078 talks about binary size in clang-cl versus VC++. On x86 builds clang-cl produces ~14% larger binaries. On x64 builds it produces ~3.5% smaller binaries. My recent changes have made the comparison tougher for clang-cl by making the VC++ binaries smaller :-)
  - brucedawson 10y ago
    I should probably take back my claim because I can't find the evidence to back it up and I might actually be wrong. I thought I remembered that there were some important cases where VC++ generated better code for Chrome, and some binary size advantages, but I can't find my references.
- Joky 10y ago

voltagex_ 10y ago

Offtopic sidenote: I wish more open source projects had instructions as good as https://www.chromium.org/developers/how-tos/debugging-on-windows https://www.chromium.org/developers/how-tos/debugging-on-win...