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

Move enum registration / conditional shard Inlining #473

Merged
merged 9 commits into from
Dec 4, 2022
Merged

Conversation

guusw
Copy link
Member

@guusw guusw commented Dec 3, 2022

Couldn't help it, I spent some extra time on improving build iteration times & cleaning up some of the enum registration.

It ties in a bit to my #472 branch too because I can move the enum help strings the source file too.

guusw added 6 commits December 3, 2022 16:34
The more logical location for registration.
Also it generally forces them to be in source files this way.

Might also make it easier to add enum help above the register function in the doc branch
@guusw
Copy link
Member Author

guusw commented Dec 3, 2022

Some statistics before/after

Full shards source

With

rm -rf src/core src/extra
time cmake --build . --target shards

devel (Debug/Windows)

cmake --build . --target shards  0.00s user 0.01s system 0% cpu 1:46.59 total

this branch (Debug/Windows)

cmake --build . --target shards  0.00s user 0.01s system 0% cpu 52.644 total
cmake --build . --target shards  0.01s user 0.00s system 0% cpu 52.190 total

Partial rebuild (gfx/gizmos)

With

pushd src/extra/CMakeFiles/shards-extra.dir; rm -rf gfx gizmos; popd
time cmake --build . --target shards

devel (Debug/Windows)

cmake --build . --target shards  0.00s user 0.00s system 0% cpu 46.512 total
cmake --build . --target shards  0.00s user 0.00s system 0% cpu 46.027 total

this branch (Debug/Windows)

cmake --build . --target shards  0.00s user 0.00s system 0% cpu 20.348 total

@guusw guusw self-assigned this Dec 3, 2022
@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Merging #473 (478a339) into devel (49da721) will decrease coverage by 0.02%.
The diff coverage is 91.02%.

@@            Coverage Diff             @@
##            devel     #473      +/-   ##
==========================================
- Coverage   81.11%   81.09%   -0.03%     
==========================================
  Files         215      218       +3     
  Lines       28694    28735      +41     
==========================================
+ Hits        23275    23302      +27     
- Misses       5419     5433      +14     
Impacted Files Coverage Δ
src/core/shards/bigint.cpp 95.76% <ø> (ø)
src/core/shards/flow.hpp 88.00% <ø> (ø)
src/core/shards/genetic.hpp 81.35% <ø> (ø)
src/core/shards/process.cpp 38.46% <ø> (ø)
src/core/shards/wasm.cpp 49.84% <ø> (ø)
src/core/shards/wires.hpp 93.54% <ø> (ø)
src/core/shards/ws.cpp 85.06% <ø> (ø)
src/extra/gfx/shader/flow_shards.cpp 86.90% <ø> (ø)
src/extra/gfx/shader/translator_utils.hpp 56.91% <ø> (ø)
src/extra/gfx/shards_types.hpp 100.00% <ø> (ø)
... and 26 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sinkingsugar
Copy link
Member

Well first of all I don't think this was a priority at all, second while nice on paper, I don't think shards will be inlined, C++ works in a very rudimentary way, inline functions have to be headers, anything .cpp gets it's own compilation unit. LTO might work but that's another bet.
Check assembly, I'm pretty sure this doesn't work.

Also you could have split the work.. enum stuff easy to approve, core runtime nope.

@sinkingsugar
Copy link
Member

Maybe you should revert just the inline shards bit, that require valgrind measuring (that's how I implemented them...) before any refactor. 👍

@sinkingsugar
Copy link
Member

sinkingsugar commented Dec 4, 2022

Alright let's review it as is since you checked asm and get it out of the way then 👍
Btw you are already in SG timezone I can see @guusw 😄

@sinkingsugar
Copy link
Member

Let me know when ready to review, add me 🙏

@guusw guusw requested a review from sinkingsugar December 4, 2022 02:22
@guusw
Copy link
Member Author

guusw commented Dec 4, 2022

Yeah should be good, ran some test script as well

(defpure inner
  = .i
  .i (Math.Add 2))

(defwire test
  0 >= .v
  [] >= .seq
  (ForRange 0 10000000 (->
                        >= .av
                        .v (Math.Add .av) > .v
                        .v (Do inner) > .v
                        .v >> .seq))
  .v)

(defmesh root)
(schedule root test)
(if (run root) nil (throw "Root tick failed"))

Same timings comparing to devel, there's not that many inline shards and this should at least touch some to indicate that they're still inlined

Copy link
Member

@sinkingsugar sinkingsugar 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 👍

@guusw guusw merged commit c3b507a into devel Dec 4, 2022
@guusw guusw deleted the guus/enum-register branch December 5, 2022 20:24
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.

2 participants