-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Use Char instead of String at escape sequences #6152
Conversation
Like an easter egg hunt, there will still have replaceable single char strings to be found, lying somewhere :-) |
@@ -39,7 +39,7 @@ class Crystal::Doc::Macro | |||
end | |||
|
|||
def anchor | |||
"#" + URI.escape(id) | |||
'#' + URI.escape(id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I'm not sure : Char + String doesn't look quite good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we should make it good if it's bad I guess.
src/compiler/crystal/compiler.cr
Outdated
print " - " | ||
print filename | ||
print ": " | ||
print " - #{filename}: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not very important in this case (not a potential tight loop) but the original 3 prints pushed 2 static strings and 1 dynamically allocated string to the output buffer, whereas your change now allocates another dynamic string and eventually pushes it to the output buffer, hence requires a thrown-away allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also made a benchmark
require "benchmark"
filename = "file.cr"
Benchmark.ips do |x|
x.report "3 prints" do
print " - "
print filename
print ": "
end
x.report "1 print" do
print " - #{filename}: "
end
end
and the 1 print is 2 times faster.
3 prints 8.35k (119.77µs) (± 6.16%) 2.30× slower
1 print 19.17k ( 52.16µs) (± 2.83%) fastest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the.... Why is that faster? @ysbaddaden you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's related to IO being flushed, if you use another function call IO stops being the bottleneck:
require "benchmark"
def print2(param); end
filename = "file.cr"
Benchmark.ips do |x|
x.report "3 prints" do
print2 " - "
print2 filename
print2 ": "
end
x.report "1 print" do
print2 " - #{filename}: "
end
end
3 prints 442.72M ( 2.26ns) (± 4.78%) fastest
1 print 6.52M (153.48ns) (± 4.70%) 67.95× slower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fastest is probably print " - ", filename, ": "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. It's actually faster.
require "benchmark"
filename = "file.cr"
Benchmark.ips do |x|
x.report "3 prints" do
3.times do
print " - "
print filename
print ": "
end
end
x.report "interpolation print" do
3.times do
print " - #{filename}: "
end
end
x.report "comma print" do
3.times do
print " - ", filename, ": "
end
end
end
(I used 3.times
to have clearer results)
3 prints 2.47k (404.48µs) (±10.79%) 2.53× slower
interpolation print 5.75k (173.88µs) (±11.99%) 1.09× slower
comma print 6.25k (160.11µs) (±13.01%) fastest
But i'm sure there are many more interpolation prints like this. Does that means every interpolation print should be changed to a comma print instead now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, use print " - ", filename, ": "
version.
I didn't used a char at the second argument (the fill argument) because later this argument gets multiplicated with * and Char doesn't supports *
@@ -25,7 +25,7 @@ private def assert_implementations(code) | |||
end | |||
end | |||
|
|||
code = code.gsub('‸', "").gsub('༓', "") | |||
code = code.delete('‸').delete('༓') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code.delete { |c| {'‸', '༓'}.includes? c }
may yield to more performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it's 2 times faster.
@@ -29,7 +29,7 @@ class Crystal::Doc::Macro | |||
|
|||
def id | |||
String.build do |io| | |||
io << to_s.gsub(/<.+?>/, "").gsub(' ', "") | |||
io << to_s.gsub(/<.+?>/, "").delete(' ') | |||
io << "-macro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
io << to_s.gsub(/<.+?>/, "").delete(' ') << "-macro"
on a single line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also do
io << to_s.gsub(/<.+?>/, "").delete(' ') \
<< "-macro"
to continue the appends to io
and not create a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm, the performance here doesn't really matters as @straight-shoota said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no performance benefit in that I don't see one style wise either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j8r Please, stop with those style-related comments. This PR is about perf improvements, not specific code style preferences. Also, such change degrades readability for no reason (other than your preferences).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here are the results
require "benchmark"
Benchmark.ips do |x|
x.report "one append to io" do
999.times do
String.build do |io|
io << "abc" << "bcd"
end
end
end
x.report "one append to io with backslash" do
999.times do
String.build do |io|
io << "abc" \
<< "bcd"
end
end
end
x.report "two appends to io" do
999.times do
String.build do |io|
io << "abc"
io << "bcd"
end
end
end
end
results:
one append to io 7.59k (131.82µs) (± 9.25%) 1.10× slower
one append to io with backslash 8.37k (119.44µs) (± 2.14%) fastest
two appends to io 8.37k (119.49µs) (± 2.43%) 1.00× slower
I'm wrong on which one is fast @Sija, but you're wrong saying that there are no speed differences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no difference, 131.82 * 0.092 = 12.12
, 131.82 - 12.12 = 119.7
I mean, do actually think about what you're doing here, the backslash is gone after the parser phase, it doesn't even affect semantic phase, let alone codegen. So the superficial difference your test shows is between the two variants that are exactly the same and just style, yet the one where there actually is a difference (can optimize out the self return by reusing the local), your tests shows there's no practical one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your benchmark result is clearly wrong because the first two examples produce exactly the same AST and LLVM IR. If one is faster than the other, that's because of external influences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're all the same even in the results, no point in even arguing about environmental issues these micro benchmarks have.
@@ -29,8 +29,7 @@ class Crystal::Doc::Macro | |||
|
|||
def id | |||
String.build do |io| | |||
io << to_s.gsub(/<.+?>/, "").gsub(' ', "") | |||
io << "-macro" | |||
io << to_s.gsub(/<.+?>/, "").delete(' ') << "-macro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulling << "-macro"
into the previous line is an unrelated change. It doesn't really improve performance like the rest.
And I find it is easier to read on a separate line.
Can we please stop with the PRs trying to change such trivial stuff as this, it likely provides no measurable performance impact and isn't the most productive use of anyone's time. |
I think this can be merged now. Or did anyone else noticed something that should maybe be improved? |
Oh all the checks failed. But thats probably because CI isn't using 0.25.0 yet. |
Please remove the unrelated commit and create a new PR for that. |
@r00ster91 Why not simply reset the branch to 4fafe42? |
@straight-shoota I'm not very experienced with git yet. I tried resetting to HEAD~{1} or something but it didn't really worked so I changed it back by hand. But next time I will remember to just specify the commit ID. Thanks. |
I personally don't think such cosmetic changes are needed or have any benefit. Maybe they improve the performance (or not) of a small part of the program that's not a bottleneck. But it does distract developers and contributors to comment and argue on these small things, instead of having them focus on more important things (be it Crystal-related or not). |
Okay I will stop now with these cosmetic changes. |
Closing since there are conflicts. And the last comment states a stop in this changes. I notice they were some changes in doc also, if they are wanted due to readability feel free to open a separate PR. @r00ster91 please note that your contributions are very welcome and I hope you will keep them coming :-) |
Too bad, this PR was not that far from a merge. |
I'd suggest finishing and merging this PR. Cosmetic or not, all in all they improve code readability and performance so they worthwhile enough. |
While they do improve performance (imperceptibly) and readability (imperceptibly), these PRs use up review bandwidth as well. You're free to make them, but I don't think it's worth my time to review such PRs too, especially when they end up with 14 commits, 10 reviews, and 33 comments such as this one. |
And some other improvements like using String#delete instead of String#gsub when it's something like
gsub(' ', "")
.