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

refactor check_all_implemented. NFC #8453

Merged

Conversation

waterlike86
Copy link
Contributor

@waterlike86 waterlike86 commented Apr 17, 2019

  • return if we do not check for undefined_symbols
  • refactor check_all_implemented
  • removed unused function is_already_implemented

possible fix for #8447

Zheng Tao Lee added 2 commits April 17, 2019 11:37
- return if we do not check for undefined_symbols
- removed unused function
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Looks good with very minor comments, thanks!

How much does this affect compile time for you?

emscripten.py Outdated
if not shared.Settings.ERROR_ON_UNDEFINED_SYMBOLS and not shared.Settings.WARN_ON_UNDEFINED_SYMBOLS:
return
# only interested in those which are not in all_implemented
notinImplemented = list(set(shared.Settings.ORIGINAL_EXPORTED_FUNCTIONS) - set(all_implemented))
Copy link
Member

Choose a reason for hiding this comment

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

how about missing as the name for this variable? and the comment could be "the initial list of missing functions are those we expected to export, but were not implemented in compiled code".

@sbc100
Copy link
Collaborator

sbc100 commented Apr 18, 2019

If this is a pure refactor, can you add ". NFC" to the end of the PR title? (that stands for Non-Functional Change).

I know we don't always do this for emscripten but its a nice convention from llvm that I've love to see used more here.

@waterlike86 waterlike86 changed the title refactor check_all_implemented refactor check_all_implemented - NFC Apr 19, 2019
@waterlike86
Copy link
Contributor Author

waterlike86 commented Apr 19, 2019

How much does this affect compile time for you?

oh this is totally skipped now in dynamic linking because of the early return. what used to take 25mins
for us now takes 9.

running this function used to take 15 mins, when ORIGINAL_EXPORTED_FUNCTIONS and all_implemented were both very big (i.e > 70000) now it takes 8 mins.

@waterlike86
Copy link
Contributor Author

If this is a pure refactor, can you add ". NFC" to the end of the PR title? (that stands for Non-Functional Change).

Done.

@waterlike86 waterlike86 changed the title refactor check_all_implemented - NFC refactor check_all_implemented. NFC Apr 19, 2019
@kripken
Copy link
Member

kripken commented Apr 20, 2019

Thanks @waterlike86!

Oh, actually NFC is not relevant here I think, as this isn't a pure refactor - it's an optimization (skips running unnecessary code)? I'll remove the NFC when I merge

@kripken kripken merged commit c67d5de into emscripten-core:incoming Apr 20, 2019
VirtualTim pushed a commit to VirtualTim/emscripten that referenced this pull request May 21, 2019
make it more efficient and skip it entirely when possible
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
make it more efficient and skip it entirely when possible
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.

3 participants