-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[Cxxmodules] Rely LazyFunctionAutoLoad for loading libraries #2559
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
Conversation
|
Starting build on |
|
Build failed on ubuntu16/native. Failing tests: |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
| if (m_unresolvedSymbols.empty()) | ||
| return false; | ||
|
|
||
| static std::vector<std::string> allreadyNotified; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have multiple instances of IncrementalExecutor per process. Please make this a member of IncrementalExecutor.
| if (m_unresolvedSymbols.empty()) | ||
| return false; | ||
|
|
||
| static std::vector<std::string> allreadyNotified; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling, "already"
| return false; | ||
|
|
||
| static std::vector<std::string> allreadyNotified; | ||
| // Call LazyFunctionAutoLoad to resolve symbols from a library and return its address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
diagnoseUnresolvedSymbols() is really only meant to diagnose. You probably want to use the symbol provision utility called by NotifyLazyFunctionCreators().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's symbol provision utility? Do you mean I should move this to callees of diagnoseUnresolvedSymbols()? There are several places who is calling this function, so I'm afraid that it'll be duplicate
04b22f1 to
9731952
Compare
|
Starting build on |
|
Build failed on slc6/gcc62. |
9731952 to
c4f14e4
Compare
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
|
@phsft-bot build |
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On |
|
Starting build on |
c4f14e4 to
7b31e47
Compare
|
Starting build on |
|
@phsft-bot build just on slc6/gcc62 with flags -Druntime_cxxmodules=On -Dctest_test_exclude_none=on |
|
Starting build on |
"InterestingDecl"s are decls which compiler needs to know its address. We were causing deserialization from HandleInterestingDecl to resolve the address, but given that modules have smarter way to solve symbols in LazyFunctionAutoLoad, we can use this instead. This improves modules startup time by 13Mbytes.
7b31e47 to
6d630da
Compare
|
Starting build on |
|
@yamaguchi1024 looks like we have another way to go. Closing for now, feel free to reopen if needed. |
"InterestingDecl"s are decls which compiler needs to know its address.
We were causing deserialization from HandleInterestingDecl to resolve
the address, but given that modules have smarter way to solve symbols
in LazyFunctionAutoLoad, we can use this instead.
This improves modules startup time by 13Mbytes.