Split ruby.h #2991
Split ruby.h #2991
Conversation
|
Confirmed that edge rails can properly be |
|
could you explain the PR more? Motivation etc. |
|
This is almost the same as #2711, except it applies the same thing against
|
|
I generally like this idea. I wonder if we should use Also, if we are considering this, is it also open to consider moving all source I tried to move |
|
Yes moving Re: directory name. It's okay to take |
Maybe requires more discussion, but I'd support (before Ruby 3):
What does Python do? Can you have Python 3.1, 3.3 installed at the same time? Maybe we should adopt, e.g. We should try to find what is best for users, OS distributions. |
|
I started to think we should move to our bug tracker. The discussion (is fruitful so far, but) started to divert from this particular pull request. |
|
I am happy for you to move the discussion. Maybe good to have a concrete proposal. We can discuss on slack? I am happy to help edit the proposal. |
Heh, you just realized that I silently removed all the tabs in the headers I touched in this pull request |
|
The way I think will best preserve history:
That way, git blame and other tools should be able to go through rename/whitespace expansion easily. |
|
Can you tell me if compile performance is affected? |
|
Well yes, as I mentioned in 7b8969e I see some slowdown in compilation. For instance GitHub Actions reports it compiled current master in 2m 23s. The same thing took 5m 19s for this branch. |
|
Can you get back speed using precompiled headers? |
|
I guess so... not tried yet. |
|
Did you check if incremental build is faster or slower? |
|
@ioquatix incremental builds are also slower than before. This is because right now, source codes directly include |
|
I'm not sure the path |
|
@ko1 or leave |
|
My intention to create a subdirectory is to be explicit that files under the new one are implementation details. Those files should not be considered as public APIs. Extension libraries are not expected to do something line |
shyouhei
force-pushed the
shyouhei:ruby.h
branch
2 times, most recently
from
fe56ce8
to
3256ded
... on mswin. According to 3ea6beb, it is a wrong idea to define HAVE_SYS_TIME_H in case of mingw. MakeMakefile#have_header do not know such restriction. It is up to the programmer to properly avoid such situation. :FIXME: I suspect this is rather a bug of have_header.
I am going to modify C codes. Must make sure that does not break things. This changeset adds many CI that basically just make binaries with slightly distinct options each other.
The ruby/ruby.h, our main public header, is the biggest header file except autogenerated ones under enc/unicode. It has roughly 2,500 LOC. It then includes ruby/intern.h, which is ~1,000 LOC. Also included are ruby/defines.h (~500 LOC), ruby/win32.h (~700 LOC), etc. It's too big! Nobody can understand what is going on. We cannot eliminate the contents for backward compatibility, but at least we can split it into many, small parts. I hope this improves understanding of our public APIs. This changeset is a pure refactoring that does not add or remove any single LOC, except for obvious header include guards.
Just a cosmetic update.
This header file improves consistency of macro definitions. Let's use it throughout the project.
This macro is worth defining becuase it elminates literally thousands of lines of copy&paste.
When I made internal/compilers.h I was afraid of global namespace pollutions. Later it turned out that our public header defines __has_attribute anyways. Let's stop worrying. Publicize what we already have internally. Doing so, however, is not a simple copy&paste business. We only had to consider C99 for internal headers. But for public ones, we need to make sure all other C/C++ versions work well.
Minor updates, like adding empty lines for readability.
NORETURN() was missing from the included files, so fixed it.
Reduced use of ruby/backward/2/attributes.h when only one macro from that header was used in the file. Direct use new syntax.
The file was necessary because ruby internals tend to confuse VALUE and void*. That was a bad habit of 20th century. Let's not be loose.
The header is emppty now.
It seems gcc takes more time to compile ruby than before. That also impacts on JIT. Extended timeouts for them.
Don't bother complex preprocessor macros. ST2FIX is exactly what is needed here.
Some experiments revealed that in case of GCC, there are chances for
this function to remain not inlined. That impacts runtime performance
negatively. Let us force inline the function. It was designed to be
inlined, then constant-folded.
Calculating -------------------------------------
before after
Optcarrot Lan_Master.nes 37.090 39.064 fps
Comparison:
Optcarrot Lan_Master.nes
after: 39.1 fps
before: 37.1 fps - 1.05x slower
Not the case of Clang but when compiled using GCC, RB_FL_ABLE shines so
brightly on top of perf report. This shall be inlined.
Calculating -------------------------------------
before after
Optcarrot Lan_Master.nes 39.685 43.147 fps
Comparison:
Optcarrot Lan_Master.nes
after: 43.1 fps
before: 39.7 fps - 1.09x slower
|
Now I think the branch is ready to be merged. |
shyouhei
merged commit 9e6e39c
into
ruby:master
|
Interesting PR. Is there any intention to change the set of symbols available through (this PR will likely cause me merge hell for any modification in |
Note that this needs to be done quite carefully for compatibility, because C extensions might expect it's a macro (e.g., if they |
|
Thank you for reviewing!
|
|
There has been a 30% drop in performance after this was merged. Can you investigate? |
|
@ioquatix Can you tell us your environment (OS, compiler, flags passed to |
|
It seems to affect optcarrot too. https://benchmark-driver.github.io/benchmarks/optcarrot/commits.html My setup has no specific configure flags. OS: Linux (same used in both tests) |
|
The optcarrot benchmark drop is because it doesn't specify |
|
I will try it now and report back. |
|
Ruby flags: I would say, that probably fixed the issue: |
|
I also got some strange reports: |
| static inline VALUE | ||
| RB_FL_TEST_RAW(VALUE obj, VALUE flags) | ||
| { | ||
| RUBY3_ASSERT_OR_ASSUME(RB_FL_ABLE(obj)); |
k0kubun
Apr 18, 2020
Member
I guess this is a new assertion introduced in this PR, and I'd like to share this helped to find 04e5695. Thank you!
|
@shyouhei I suspect you might like this somewhat related change in TruffleRuby: oracle/truffleruby@d83ddc3 Now it looks like there is a small MRI in https://github.com/oracle/truffleruby/tree/master/src/main/c/cext except most C functions just call back to Ruby and not the other way around. |
|
Yes, very interesting. Thank you. Let me take a closer look at them. I guess @ko1 might also be interested in it. His current approach (ruby methods are written in ruby, who call C codes via _builtin) seems like the opposite of them. |
Turn this:
into this:
