Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SELECTANY usage #39532

Merged
merged 1 commit into from
Jul 18, 2020
Merged

Remove SELECTANY usage #39532

merged 1 commit into from
Jul 18, 2020

Conversation

janvorli
Copy link
Member

@janvorli janvorli commented Jul 17, 2020

This change replaces SELECTANY by using constexpr. It results in major
reduction of the native binaries size (3.2MB in the libcoreclr.so case)

Close #39281

@janvorli janvorli added this to the 5.0.0 milestone Jul 17, 2020
@janvorli janvorli requested a review from jkotas July 17, 2020 18:03
@janvorli janvorli self-assigned this Jul 17, 2020
@jkotas
Copy link
Member

jkotas commented Jul 17, 2020

Does constexpr guarantee that there are no duplicates? I believe that it only strips unused references, but it does not eliminate duplicates, so we are still leaving money on the table. https://stackoverflow.com/questions/50488831/use-of-constexpr-in-header-file has some context.

I believe that it would have to be inline constexpr to actually eliminate duplicates.

@jkotas
Copy link
Member

jkotas commented Jul 17, 2020

Also, we should get rid of other duplicate static const data from the header files. Here are the constant data symbols that I see defined in the .rodata more than once:

        0000000000000007              _ZL12__nullstring
        000000000000000c              _ZL22UnwindOpExtraSlotTable
        000000000000000e              _ZL13__wnullstring
        0000000000000010              empty_frame
        0000000000000014              _ZN14DiagnosticsIpcL18GenericErrorHeaderE
        0000000000000014              _ZN14DiagnosticsIpcL20GenericSuccessHeaderE
        0000000000000059              _ZL15__lookuptable_s
        00000000000000a0              _ZL29g_rbTheSilverlightPlatformKey
        0000000000000100              operands
        0000000000000118              _ZN12_GLOBAL__N_114g_shash_primesE

@janvorli
Copy link
Member Author

@jkotas I was considering that, but inline constexpr is c++17 only. We currently use c++11 features and I am not sure if we want to move to c++17 just for this.

@jkotas
Copy link
Member

jkotas commented Jul 17, 2020

Agree. Should we do this the old-fashioned way and move the definitions to .c/.cpp files?

@janvorli
Copy link
Member Author

There are only a few symbols duplicated, all of them taking 16 bytes (896 bytes total). So there doesn't seem to be much to be gained by redoing the change in the traditional way.

Moreover, while some of the cases can be handled trivially, the EXTERN_GUID case would be kind of ugly, as we would have to duplicate all the GUIDS at two places (one in the header, one in a cpp file). Lot of the guids are in prebuilt headers on Unix pregenerated from ild files.

2x IID_ICeeGen
2x IID_ICeeGenInternal
2x IID_IGetIMDInternalImport
5x IID_IMapToken
6x IID_IMDCommon
3x IID_IMDInternalEmit
7x IID_IMDInternalImport
2x IID_IMDInternalImportENC
2x IID_IMetaDataAssemblyEmit
2x IID_IMetaDataDispenserEx
3x IID_IMetaDataEmit
2x IID_IMetaDataEmit2
2x IID_IMetaDataEmitHelper
2x IID_IMetaDataHelper
6x IID_IMetaDataImport
3x IID_IMetaDataImport2
2x IID_IMetaDataTables

20x g_tkCorEncodeToken

So I'd prefer keeping the change as is.

@janvorli
Copy link
Member Author

I was referring to the symbols that my change touched.

@jkotas
Copy link
Member

jkotas commented Jul 17, 2020

There are only a few symbols duplicated, all of them taking 16 bytes (896 bytes total)

Ok, that does not sound too bad. Could you please resolve the conflict?

This change replaces SELECTANY by using constexpr. It results in major
reduction of the native binaries size (3.2MB in the libcoreclr.so case)
@janvorli
Copy link
Member Author

Rebased

@jkotas
Copy link
Member

jkotas commented Jul 18, 2020

Failure is #39484

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jkotas
Copy link
Member

jkotas commented Jul 18, 2020

This change regressed coreclr.dll size on Windows by ~200kB. I have opened a new bug on this: #39599

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
This change replaces SELECTANY by using constexpr. It results in major
reduction of the native binaries size (3.2MB in the libcoreclr.so case)
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native .so files are considerably larger in 5.0 than they were in 3.1
2 participants