Generate `DecodeFrom()` methods for Golang #71

2opremio · 2021-11-11T19:58:42Z

This is the decoding counterpart of #65

There is a dramatic performance improvement: ~13x speedup and and ~11x allocation reduction.

Check stellar/go#4064 for details on the speedup and the generated code.

TODO:

Make EncodeTo()-related code consistent with the changes in this PR. I intentionally didn't touch EncodeTo-code here in order to avoid noise (otherwise it will be more difficult to read the generated code at xdr: update using xdrgen#71 go#4064 ). I will update them in a separate PR.

2opremio · 2021-11-11T20:13:41Z

It seems there is still a minor problem (decoding an empty value instead of nil in some cases). I will look into that soon

2opremio · 2021-11-11T22:43:28Z

OK, that's fixed, but verify-range seems to be failing https://app.circleci.com/pipelines/github/stellar/go/9834/workflows/44ff1f80-b9fe-4787-8783-711d6cd0b8e8/jobs/57566

2opremio · 2021-11-12T01:04:48Z

@bartekn could you please take a quick look at the failure above?

ERRO[2021-11-12T00:24:58.012Z] Error in ingestion state machine              current_state="verifyRange(fromLedger=10000063, toLedger=10000127, verifyState=true)" error="Error preparing range: Error fast-forwarding to 10000063: stellar core exited unexpectedly" next_state=stop pid=430 service=ingest
INFO[2021-11-12T00:24:58.012Z] Shut down                                     pid=430 service=ingest
Error preparing range: Error fast-forwarding to 10000063: stellar core exited unexpectedly

I don't understand why stellar core fails and there is no context.

2opremio · 2021-11-12T02:42:46Z

OK, I think I know what's happening. I shouldn't be using ReadAll()

There was a bug in the captive core backend hiding the error stellar/go#4066

2opremio · 2021-11-12T13:48:27Z

OK, the verify-range problem is fixed. I am going to run verify-range in the last 100k ledgers 🤞

lib/xdrgen/generators/go.rb

2opremio · 2021-11-12T14:50:38Z

Note for reviewers, please check the code generated at stellar/go#4064 before reviewing the code here.

In fact, it's vital to review the code generated before reviewing this PR itself.

2opremio · 2021-11-12T17:21:51Z

verify range succeeded! 🥳

Status of auto-second-half-job28279:
{
  "STARTING": 0,
  "FAILED": 0,
  "RUNNING": 0,
  "SUCCEEDED": 9,
  "RUNNABLE": 0,
  "SUBMITTED": 0,
  "PENDING": 0
}

lib/xdrgen/generators/go.rb

paulbellamy · 2021-11-15T09:47:58Z

lib/xdrgen/generators/go.rb

+            out.puts "  for i := 0; i < len(#{var}); i++ {"
+            element_var = "#{var}[i]"
+            optional_within = type.is_a?(AST::Identifier) && type.resolved_type.sub_type == :optional
+            if optional_within


Intuitively, it seems like this optional block (and above) should be able to be handled with a recursive data-type, like a Option<Foo> type-thing, and having a render_optional method... 🤔 but this is probably good enough for now.

I think I'm thinking something like:

def render_decode_from_body(out, var, type, declared_variables:, self_encode:) # ... if type.sub_type == :optional return render_optional(out, var, type, type, declared_variables: declared_variables, self_encode: self_encode) do |inner_type| # Recurse into the unwrapped inner type render_decode_from_body(out, var, inner_type, declared_variables: declared_variables, self_encode: self_encode) end end # ... end def render_optional(out, var, type, declared_variables:, self_encode:) render_variable_declaration(out, " ", 'b', "bool", declared_variables: declared_variables) out.puts " b, nTmp, err = d.DecodeBool()" out.puts tail out.puts " #{var} = nil" out.puts " if b {" out.puts " #{var} = new(#{name type.resolved_type.declaration.type})" # TODO: Unwrap the inner_type (without the optional wrapper) here yield inner_type out.puts " }" end

But, actually you could do a similar block thing for arrays as well, I think...

Either way, probably outside the scope of this PR.

paulbellamy · 2021-11-15T10:20:12Z

also, unrelated to this PR, but wdyt of, instead of doing puts " some spaces for indentation", just doing puts "no indentation spaces", and running gofmt on it at the end?

2opremio · 2021-11-15T12:40:09Z

also, unrelated to this PR, but wdyt of, instead of doing puts " some spaces for indentation", just doing puts "no indentation spaces", and running gofmt on it at the end?

I think it's probably a good idea. we in fact do run gofmt when generating the code in stellar-go. However, that requires a bigger refactoring

2opremio · 2021-11-15T12:40:17Z

@bartekn PTAL

bartekn

Awesome!

leighmcculloch

Three requests (❗) otherwise LGTM. I definitely don't understand all of it, unfortunately xdrgen is pretty complex, but the areas of the generated code I sampled in stellar/go#4064 look good.

I also left some non-blocking comments: a suggestion (💡), a thought for future improvements that may or may not be beneficial (💭), and a question about something I don't understand (❔).

🔍 I see above we ran a verify range test with 100k ledgers. That's only 5 days worth of ledgers. Given the larger surface area of xdr decoding and its risk for Horizon, and that Horizon supports reingestion of much larger periods of time, should we perform a verify range of a much larger range of time before merging? cc @bartekn

leighmcculloch · 2021-11-15T17:11:04Z

lib/xdrgen/generators/go.rb

@@ -580,8 +815,16 @@ def render_top_matter(out)
            xdrType()
          }

+          type xdrDecodable interface {


❗ Could you add for every type a cast to an unnamed variable? We do that with all the other interfaces in here to make sure all the types satisfy the interfaces. i.e.:

var _ xdrDecodeable = (*#{name})(nil)

❗ Could you name this interface decoderFrom? That's the pattern used for similar named interfaces in the stdlib. e.g. The interface that contains the ReadFrom function is ReaderFrom.

+1 As a follow up we can remove the xdrType thing I added too, it's superfluous and wasn't the best approach I took. I'm happy to do the clean up for EncodeTo, but if you're going to do it 👍🏻 sgtm.

If you could cleanup the encodeto part (adding errors, making it consistent with this code etc ...). I would appreciate it.

leighmcculloch · 2021-11-15T17:26:12Z

lib/xdrgen/generators/go.rb

+          if err != nil {
+            return n, err
+          }
+          if _, ok := #{private_name type}Map[v]; !ok {


💭 Given that most of our enums start at zero and are sequential, and given that use of the map will now be in the hot path, I wonder if for enums that we detect have that property, if we could simply replace this with a if v >= len(...Map) { // error }? We don't have to do this, and we definitely don't have to do this in this PR, but maybe something to evaluate if there is a saving here over hitting the map.

Good idea, However:

There are many enums with negative values. Some examples are: BucketEntryType, CreateAccountResultCode, PaymentResultCode, PathPaymentStrictReceiveResultCode ....

Not even a range is generally a possibility since the XDR definition can contain scattered values.

We could infer a range where available and use that, but I think it is an overoptimization (without further proof of a bottleneck) and definitely out of the scope of this PR.

Let's later see if further profiling reveals map access as a problem.

leighmcculloch · 2021-11-15T17:30:20Z

lib/xdrgen/generators/go.rb

+              render_variable_declaration(out, "  ", 'b', "bool", declared_variables: declared_variables)
+              out.puts "  b, nTmp, err = d.DecodeBool()"
+              out.puts tail
+              out.puts "  #{var} = nil"


🎉 I really like that this sets the value of every field.

lib/xdrgen/generators/go.rb

2opremio mentioned this pull request Nov 11, 2021

xdr: update using xdrgen#71 stellar/go#4064

Merged

2opremio requested review from leighmcculloch and bartekn November 11, 2021 20:09

bartekn reviewed Nov 12, 2021

View reviewed changes

lib/xdrgen/generators/go.rb Outdated Show resolved Hide resolved

2opremio force-pushed the generate-decodefrom branch 2 times, most recently from 966e0f4 to 968464a Compare November 12, 2021 17:38

2opremio mentioned this pull request Nov 14, 2021

services/horizon: Use encoding buffer in state verifier stellar/go#4069

Merged

paulbellamy reviewed Nov 15, 2021

View reviewed changes

paulbellamy approved these changes Nov 15, 2021

View reviewed changes

bartekn approved these changes Nov 15, 2021

View reviewed changes

leighmcculloch approved these changes Nov 15, 2021

View reviewed changes

2opremio added 9 commits November 15, 2021 21:32

WIP: generate DecodeFrom() Go methods

c3262c3

Progress

acc19fc

Do not return an old error

11075ae

Error on invalid input discriminants

127b211

Do not initialize 0-length slices and remove redundant error checks

70b912e

Resolve remaining TODOs

1968364

Remove ReadAll and modify DecodeFrom() to returned consumed length

e72592d

Increment read counter before checking for the error

ccbd4d3

Refine slice limit error

101c6a6

2opremio added 2 commits November 15, 2021 21:32

Remove redundant error returns for unions

b6855aa

Ensure decoded enum values are valid

548f250

2opremio force-pushed the generate-decodefrom branch from 968464a to 548f250 Compare November 15, 2021 21:50

Address review feedback

c077a53

2opremio merged commit 7d53fb1 into master Nov 15, 2021

2opremio deleted the generate-decodefrom branch November 15, 2021 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate `DecodeFrom()` methods for Golang #71

Generate `DecodeFrom()` methods for Golang #71

2opremio commented Nov 11, 2021 •

edited

Loading

2opremio commented Nov 11, 2021 •

edited

Loading

2opremio commented Nov 11, 2021 •

edited

Loading

2opremio commented Nov 12, 2021 •

edited

Loading

2opremio commented Nov 12, 2021

2opremio commented Nov 12, 2021

2opremio commented Nov 12, 2021

2opremio commented Nov 12, 2021

paulbellamy Nov 15, 2021

paulbellamy Nov 15, 2021 •

edited

Loading

paulbellamy Nov 15, 2021

paulbellamy commented Nov 15, 2021 •

edited

Loading

2opremio commented Nov 15, 2021

2opremio commented Nov 15, 2021

bartekn left a comment

leighmcculloch left a comment •

edited

Loading

leighmcculloch Nov 15, 2021

leighmcculloch Nov 15, 2021

2opremio Nov 15, 2021

leighmcculloch Nov 15, 2021

2opremio Nov 15, 2021 •

edited

Loading

leighmcculloch Nov 15, 2021

2opremio Nov 15, 2021

leighmcculloch Nov 15, 2021

Generate DecodeFrom() methods for Golang #71

Generate DecodeFrom() methods for Golang #71

Conversation

2opremio commented Nov 11, 2021 • edited Loading

2opremio commented Nov 11, 2021 • edited Loading

2opremio commented Nov 11, 2021 • edited Loading

2opremio commented Nov 12, 2021 • edited Loading

2opremio commented Nov 12, 2021

2opremio commented Nov 12, 2021

2opremio commented Nov 12, 2021

2opremio commented Nov 12, 2021

Choose a reason for hiding this comment

paulbellamy Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulbellamy commented Nov 15, 2021 • edited Loading

2opremio commented Nov 15, 2021

2opremio commented Nov 15, 2021

bartekn left a comment

Choose a reason for hiding this comment

leighmcculloch left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Generate `DecodeFrom()` methods for Golang #71

Generate `DecodeFrom()` methods for Golang #71

2opremio commented Nov 11, 2021 •

edited

Loading

2opremio commented Nov 11, 2021 •

edited

Loading

2opremio commented Nov 11, 2021 •

edited

Loading

2opremio commented Nov 12, 2021 •

edited

Loading

paulbellamy Nov 15, 2021 •

edited

Loading

paulbellamy commented Nov 15, 2021 •

edited

Loading

leighmcculloch left a comment •

edited

Loading

2opremio Nov 15, 2021 •

edited

Loading