-
Notifications
You must be signed in to change notification settings - Fork 115
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
Avoid dynamic parse method dispatch for faster access #311
Conversation
benchmark/convert_nil.yaml
Outdated
csv: 3.0.1 | ||
- gems: | ||
csv: 3.0.2 |
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.
We should not remove old versions because we use them as base 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.
@kou Done
378f55f
to
dedf2e5
Compare
lib/csv/parser.rb
Outdated
else | ||
parse_quotable_loose(&block) | ||
end | ||
@parse_method.call(&block) |
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.
Could you use :parse_XXX
and __send__
instead of method(:parse_XXX)
and call
?
@parse_method.call(&block) | |
__send__(@parse_method, &block) |
lib/csv/parser.rb
Outdated
@parse_method = if @quote_character.nil? | ||
method(:parse_no_quote) | ||
elsif @liberal_parsing || @strip | ||
method(:parse_quotable_robust) | ||
else | ||
method(:parse_quotable_loose) | ||
end |
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.
Could you use the existing style?
@parse_method = if @quote_character.nil? | |
method(:parse_no_quote) | |
elsif @liberal_parsing || @strip | |
method(:parse_quotable_robust) | |
else | |
method(:parse_quotable_loose) | |
end | |
if @quote_character.nil? | |
@parse_method = :parse_no_quote | |
elsif @liberal_parsing or @strip | |
@parse_method = :parse_quotable_robust | |
else | |
@parse_method = :parse_quotable_loose | |
end |
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.
Could you check this comment?
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.
Oh I missed or
idea
lib/csv/parser.rb
Outdated
@@ -987,7 +984,7 @@ def parse_quotable_loose(&block) | |||
quoted_fields = [] | |||
elsif line.include?(@cr) or line.include?(@lf) | |||
@scanner.keep_back | |||
@need_robust_parsing = true | |||
@parse_method = method(:parse_quotable_robust) |
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.
@parse_method = method(:parse_quotable_robust) | |
@parse_method = :parse_quotable_robust |
lib/csv/parser.rb
Outdated
@@ -1011,7 +1008,7 @@ def parse_quotable_loose(&block) | |||
row[i] = column[1..-2] | |||
else | |||
@scanner.keep_back | |||
@need_robust_parsing = true | |||
@parse_method = method(:parse_quotable_robust) |
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.
@parse_method = method(:parse_quotable_robust) | |
@parse_method = :parse_quotable_robust |
dedf2e5
to
19d27b7
Compare
Could you benchmark the |
@kou they appear to perform the same but I agree that
|
19d27b7
to
0059338
Compare
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.
Could you update the description to the latest information before we merge this?
lib/csv/parser.rb
Outdated
@parse_method = if @quote_character.nil? | ||
:parse_no_quote | ||
elsif @liberal_parsing or @strip | ||
:parse_quotable_robust | ||
else | ||
:parse_quotable_loose | ||
end |
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.
@parse_method = if @quote_character.nil? | |
:parse_no_quote | |
elsif @liberal_parsing or @strip | |
:parse_quotable_robust | |
else | |
:parse_quotable_loose | |
end | |
if @quote_character.nil? | |
@parse_method = :parse_no_quote | |
elsif @liberal_parsing or @strip | |
@parse_method = :parse_quotable_robust | |
else | |
@parse_method = :parse_quotable_loose | |
end |
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.
done
0059338
to
d5e8134
Compare
Thanks. |
On some benchmarks it seems to make a difference:
quoted
frombenchmark/parse.yaml
quote_char_nil
frombenchmark/parse_quote_char_nil.yaml