Skip to content

Revert "Save 908,288 bytes by deleting 'const' three times"#14

Open
randomascii wants to merge 1 commit intogoogle:masterfrom
randomascii:revert_const_hack
Open

Revert "Save 908,288 bytes by deleting 'const' three times"#14
randomascii wants to merge 1 commit intogoogle:masterfrom
randomascii:revert_const_hack

Conversation

@randomascii
Copy link
Copy Markdown
Contributor

This reverts commit 9a5abb8. VS is
still working on a fix to the underlying issue but clang-cl doesn't need
this hack so we can remove it.

Bug: 934323

This reverts commit 9a5abb8. VS is
still working on a fix to the underlying issue but clang-cl doesn't need
this hack so we can remove it.

Bug: 934323
@randomascii
Copy link
Copy Markdown
Contributor Author

For full context, I created the original PR in order to work around a VC++ code-gen deficiency. Full details are here:

https://chromium.googlesource.com/external/github.com/google/compact_enc_det.git/+/9a5abb86e339beca2ad7f375934a38727e810f45

The summary is that const struct/class members confuse VC++ when generating const global objects. It prevents the objects from going in the read-only segment and in this case also cause tons of code to be generated. The original PR was a big win for Chrome.

But Chrome doesn't build with VC++ anymore, so this is no longer needed for Chrome.

The point of discussion is that the VC++ bug has not yet been fixed. It was forgotten. It's being looked at for VS 2019 (https://twitter.com/TartanLlama/status/1099960433677647874) but, who knows?

So, whether to accept this PR comes down to whether VS is still an important customer, and how the project feels about hacky workarounds. It would be possible to #ifdef the changes of course, if that is wanted.

So that's where we are. crbug.com/934323 tracks removing these two-year-old hacks from Chrome.

@JinsukKim
Copy link
Copy Markdown
Contributor

Thanks for the patch.

The original patch is still beneficial for those using VC++ build by generating a smaller binary. I think it's worth keeping till VC++ fixes their bug. I'd either keep it or see it ifdef'd.

@randomascii
Copy link
Copy Markdown
Contributor Author

Makes sense. #ifdefing is ugly but would be a self-documenting way of dealing with it. Alternately I've asked the VC++ team to revisit this when they ship a fix.
https://twitter.com/BruceDawson0xB/status/1100577460179988481

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants