Use os.execv when we only running clang to compile. NFC#20884
Merged
Conversation
747eb01 to
df1abf8
Compare
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 8, 2023
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. emscripten-core#20884)
RReverser
reviewed
Dec 8, 2023
|
|
||
|
|
||
| def exec_subprocess_and_exit(cmd): | ||
| if utils.WINDOWS: |
Collaborator
There was a problem hiding this comment.
I don't think this is necessary - Python has execv on Windows too, it just can't load cmd into the same process, so it falls back to the slower path automatically.
Might be cleaner to just use the same API on both platforms.
Collaborator
Author
There was a problem hiding this comment.
Oh I didnt know they had emulation. Thats odd.
It looks like there are maybe some issues with the emulation though: https://bugs.python.org/issue19066.. those seems like fairly major issues, but I guess it would be easy enough to detect and our tests would fail I believe.
Collaborator
There was a problem hiding this comment.
Oh huh, if there are issues, it's probably not worth it. I thought it would be cleaner and easier to just use single API, but if it doesn't fall back to whatever subprocess does, that's a moot point.
Collaborator
There was a problem hiding this comment.
Although that one is from 2014 and was closed, so maybe not a problem anymore?
Collaborator
Author
There was a problem hiding this comment.
Yes, I have to land a few other changes before this one (e.g. #20886), but if the tests all pass with execv on windows then I'll go with that option.
Collaborator
There was a problem hiding this comment.
Looks like you left this in?
Collaborator
Author
There was a problem hiding this comment.
yeah, I figured we can try and take it out after and see what CI says. I guess I could have tried it right away..
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 8, 2023
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. emscripten-core#20884)
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 8, 2023
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. emscripten-core#20884)
kripken
approved these changes
Dec 11, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 11, 2023
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. emscripten-core#20884)
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 11, 2023
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. emscripten-core#20884)
sbc100
added a commit
that referenced
this pull request
Dec 12, 2023
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. #20884)
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 12, 2023
This saves about 10ms of overhead on my linux machine which is about 10% of the overall overhead or emcc.py over base clang. It will also save on memory since python no longer needs to be resident at the same time as clang.
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 13, 2023
Followup to emscripten-core#20884
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 13, 2023
Followup to emscripten-core#20884
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Dec 13, 2023
Followup to emscripten-core#20884
sbc100
added a commit
that referenced
this pull request
Dec 13, 2023
sbc100
added a commit
to sbc100/emscripten
that referenced
this pull request
Jan 3, 2025
We had this rather strange (I imagine completely unused) feature where is emscripten is passed a concrete path to a shared library file (e.g. /path/to/libfoo.so) and it could determine that the shared library was not indented for emscripten then it would instead search the library for other things called `libfoo` that might work. This seems too magical at best and harmful/confusing a worst. The presence of this feature is blocking the way for some internal optimizations and refactorings (e.g. emscripten-core#20884)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This saves about 10ms of overhead on my linux machine which is about 10% of the overall overhead or emcc.py over base clang.
It will also save on memory since python no longer needs to be resident at the same time as clang.