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

Tweak generated class methods .lookup and .resolve for enums to prevent StackError when formatting #172

Merged
merged 1 commit into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 49 additions & 27 deletions lib/protoboeuf/codegen.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,53 @@ def initialize(enum, generate_types:, options: {})
end

def result
"module #{enum.name}\n" + class_body + "; end\n"
<<~RESULT
module #{enum.name}
#{values}

#{lookup}

#{resolve}
end
RESULT
end

private

def class_body
enum.value.map { |const|
"#{const.name} = #{const.number}"
}.join("\n") + "\n\n" + lookup + "\n\n" + resolve
VALUES = ERB.new(<<~VALUES, trim_mode: "-")
<%- enum.value.each do |const| -%>
<%= const.name %> = <%= const.number %>
<%- end -%>
VALUES

def values
VALUES.result(binding)
end

LOOKUP = ERB.new(<<~LOOKUP, trim_mode: "-")
Copy link

@nirvdrum nirvdrum Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is just improving upon what we already have, but I wonder if we could use Hash here or something else with constant-time access. As implemented, we're looking at linear-time lookups. I could see this potentially being faster if enum.value was a frequency list with an infrequently accessed tail, but I think in practice it's just the order in the protobuf definition.

@tenderlove Were the values enumerated as a long list of branches for YJIT?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're generating the code, we could do a mixed strategy, too (assuming the current approach is to optimize on YJIT). E.g., if enum.value.size < 10 or whatever, emit the chain of conditionals, otherwise store in a Hash and do a lookup that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can merge this as-is then followup with that. I created an issue: #176

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tenderlove Were the values enumerated as a long list of branches for YJIT?

Yes, they were. The idea was that the enums were probably short enough that staying in machine code would outperform calling out to a hash. I thought at one point we'd used a sparse array though? I feel you could do this with an array, but the catch is that Protobuf allows negative enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I updated the Issue title to "Investigate using a Hash or sparse Array for Enum lookup and resolve instead of a series of conditonals"

<%= type_signature(params: { val: "Integer" }, returns: "Symbol", newline: true) %>
def self.lookup(val)
<%- enum.value.each do |const| -%>
return :<%= const.name %> if val == <%= const.number %>
<%- end -%>
end
LOOKUP

def lookup
type_signature(params: { val: "Integer" }, returns: "Symbol", newline: true) +
"def self.lookup(val)\n" \
"if " + enum.value.map { |const|
"val == #{const.number} then :#{const.name}"
}.join(" elsif ") + " end; end"
LOOKUP.result(binding)
end

RESOLVE = ERB.new(<<~RESOLVE, trim_mode: "-")
<%= type_signature(params: { val: "Symbol" }, returns: "Integer", newline: true) %>
def self.resolve(val)
<%- enum.value.each do |const| -%>
return <%= const.number %> if val == :<%= const.name %>
<%- end -%>
end
RESOLVE

def resolve
type_signature(params: { val: "Symbol" }, returns: "Integer", newline: true) +
"def self.resolve(val)\n" \
"if " + enum.value.map { |const|
"val == :#{const.name} then #{const.number}"
}.join(" elsif ") + " end; end"
RESOLVE.result(binding)
end
end

Expand Down Expand Up @@ -699,25 +721,25 @@ def enum_writers
return "" if fields.empty?

"# enum writers\n" +
fields.map { |field|
fields.map do |field|
"def #{field.name}=(v); #{iv_name(field)} = #{enum_name(field)}.resolve(v) || v; end"
}.join("\n") + "\n\n"
end.join("\n") + "\n\n"
end

def required_writers
fields = @required_fields

return "" if fields.empty?

fields.map { |field|
fields.map do |field|
<<~RUBY
#{type_signature(params: { v: convert_field_type(field) })}
def #{field.name}=(v)
#{bounds_check(field, "v")}
#{iv_name(field)} = v
end
RUBY
}.join("\n") + "\n"
end.join("\n") + "\n"
end

def optional_writers
Expand All @@ -741,20 +763,20 @@ def oneof_writers
return "" if oneof_fields.empty?

"# BEGIN writers for oneof fields\n" +
oneof_fields.map.with_index { |sub_fields, i|
oneof_fields.map.with_index do |sub_fields, i|
next unless sub_fields

oneof = message.oneof_decl[i]
sub_fields.map { |field|
sub_fields.map do |field|
<<~RUBY
def #{field.name}=(v)
#{bounds_check(field, "v")}
@#{oneof.name} = :#{field.name}
#{iv_name(field)} = v
end
RUBY
}.join("\n")
}.join("\n") +
end.join("\n")
end.join("\n") +
"# END writers for oneof fields\n\n"
end

Expand All @@ -773,9 +795,9 @@ def initialize_code
end

def initialize_oneofs
@oneof_selection_fields.map { |field|
@oneof_selection_fields.map do |field|
"#{iv_name(field)} = nil # oneof field"
}.join("\n") + "\n"
end.join("\n") + "\n"
end

def initialize_oneof(field, msg)
Expand Down Expand Up @@ -915,14 +937,14 @@ def to_proto(_options = {})
def optional_predicates
return "" if optional_fields.empty?

optional_fields.map { |field|
optional_fields.map do |field|
<<~RUBY
#{type_signature(returns: "T::Boolean")}
def has_#{field.name}?
#{test_bitmask(field)}
end
RUBY
}.join("\n") + "\n"
end.join("\n") + "\n"
end

def decode
Expand Down
56 changes: 18 additions & 38 deletions lib/protoboeuf/google/api/field_behavior.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading