[finch] Remove the three-argument version of BASE_FEATURE from features_generated.cc. [chromium/src : main]
Alexei Svitkine (Gerrit)
Alexei Svitkine voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Julia Hansbrough
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
Hmm, this is showing as a binary size increase, which is surprising to me. I thought the 2-arg macro would generate the same code as the 3-arg one.
Related details
- Julia Hansbrough
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
Hmm, this is showing as a binary size increase, which is surprising to me. I thought the 2-arg macro would generate the same code as the 3-arg one.
Finally had a chance to look into this!
Full disclosure: I used Gemini to help me root-cause this. I've checked its logic & checked against my own build (& the writeup here is my own), but feel free to speak up if I've missed something:
It looks like the issue here is a change introduced in crrev.com/6903879: while using StringStorage to derive a feature name does save us one character's worth of .rodata, but emitting that helper lambda for each Feature *adds* to `.text` (and the accompanying symbol table entry, etc).
Back when that CL went in, not many things were using the two-argument macro, and the migrations over since then have been piecewise, and thus (presumably) too small of changes to get dinged by the binary size checker. But changing so many Features *all at once* I think is what's causing the problem here.
Replacing `StringStorage` with the `string_view(#feature).substr(1).data()` seems to achieve the same functionality without making the binary size checker unhappy. So, perhaps we can simply merge it with this change? Please take a look and let me know (leaving this comment unresolved until I've got a +1 from you).
Related details
- Alexei Svitkine
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |