Move instantiation to separate file #518
Merged
Conversation
This file contains 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
gumb0
force-pushed
the
instantiate-hpp
branch
3 times, most recently
from
b05a2aa
to
03d4343
Compare
Codecov Report
@@ Coverage Diff @@
## master #518 +/- ##
=======================================
Coverage 98.73% 98.73%
=======================================
Files 54 56 +2
Lines 8695 8695
=======================================
Hits 8585 8585
Misses 110 110 |
|
Weird, that I can't reproduce clang-tidy warning locally, using clang 10.0.1... |
chfast
reviewed
Sep 4, 2020
lib/fizzy/instantiate.cpp
Outdated
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| #include "instantiate.hpp" | ||
| #include "execute.hpp" |
There was a problem hiding this comment.
Why? Looks like cyclic dependency.
There was a problem hiding this comment.
There are calls to execute when initializing table elements etc.
There was a problem hiding this comment.
Ugh. Can you leave a comment perhaps?
No. I only tried to remove unnecessary |
The only change is suppressing clang-tidye in the last commit. |
axic
reviewed
Sep 4, 2020
lib/fizzy/execute.cpp
Outdated
| @@ -685,142 +469,6 @@ std::optional<uint32_t> find_export(const Module& module, ExternalKind kind, std | |||
|
|
|||
There was a problem hiding this comment.
Where is the find_export helper above used? I always thought it is used by instantiation, but it is used in the execution loop?!
There was a problem hiding this comment.
No, it's used only in find_exported_* API, which stays in execute.cpp for now
There was a problem hiding this comment.
Those operate on the instance, shouldn't they belong into instantiate.cpp?
There was a problem hiding this comment.
Logically they are neither instantiation nor execution, I would put them into separate header, maybe export.hpp, or api.hpp because we have api_test.cpp for them. What do you think?
There was a problem hiding this comment.
It is hard for me to discriminate what they are for (not my area of knowledge) so "kinda whatever". Can be also "module.hpp/cpp" if that make sense (I know this is non-functional name, but this can be found in other projects).
But I really want to have only execute() function in execute.cpp what makes execution optimizations easier to conduct. In the end we can think about it as: extracting execute() from the instatiate.cpp file...
There was a problem hiding this comment.
That was getting a bit too complicated for one PR, will keep them in instantiate.
|
This actually makes execution significant faster (GCC10, no-LTO): |
chfast
requested changes
Sep 4, 2020
|
But no much change for LTO builds: |
gumb0
force-pushed
the
instantiate-hpp
branch
2 times, most recently
from
cd55a7d
to
eb6b556
Compare
|
No clang-tidy warning again. |
chfast
approved these changes
Sep 7, 2020
|
Now some benchmarks gets slower. LTO non-LTO |
|
My results are mostly within 1-2% change GCC10, LTO GCC10, non-LTO |
|
For me this is still good to merge this. |
gumb0
force-pushed
the
instantiate-hpp
branch
2 times, most recently
from
47321bd
to
06b5b47
Compare
|
@gumb0 sorry for being late to the party, but the first commit renames execute to instantiate, which in practice means we value the history of instantiate code lines more, than the execute lines. I would argue the history of So I suggest we do the inverse, and keep the |
|
@axic That rename is only in the branch, the final merge commit resolves it in a way that history is kept for both instantiate.* and execute.*, check it locally. |
gumb0
force-pushed
the
instantiate-hpp
branch
from
06b5b47
to
0ffccc2
Compare
The following steps allow us to keep the history of both execute.* and instantiate.*: 1. In branch1 rename execute.* -> instantiate.* (git's history will consider instantiate to be the continuation of execute) 2. In branch1 remove execution code from instantiate.* 3. In branch2 rename instantiation code from execute.* 4. Merge branch1 into branch2.
gumb0
force-pushed
the
instantiate-hpp
branch
from
0ffccc2
to
48fd8f8
Compare
axic
approved these changes
Sep 9, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 tries to keep git blame results clear, therefore needs a merge commit for that.
Suppressing clang-tidy false positive warning was added first in aba1d78, then was removed in #314, now it looks to be needed again.