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

xdr: update using stellar/xdrgen#68 #4063

Merged
merged 1 commit into from
Nov 11, 2021
Merged

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Nov 10, 2021

It improves XDR marshaling performance.

See stellar/xdrgen#68 for details

@2opremio 2opremio requested review from leighmcculloch and a team November 10, 2021 22:18
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Two requests (❗), otherwise looks great. 🎉

@@ -40,7 +40,6 @@ namespace :xdr do
require "pathname"
require "xdrgen"
require 'fileutils'
FileUtils.rm_f("xdr/xdr_generated.go")
Copy link
Member

Choose a reason for hiding this comment

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

❗ This looks unrelated and like an unnecessary change. Why is the change needed? I believe this is there to make sure that any xdr_generated.go we have at the end of a generate run is new and no silent error that doesn't result in a new file being written goes undetected.

Copy link
Contributor Author

@2opremio 2opremio Nov 10, 2021

Choose a reason for hiding this comment

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

It made development unpleasant because my IDE closed the buffer when the file was removed.

There is no compelling reason to remove the file, since it's overwritten.

Copy link
Member

Choose a reason for hiding this comment

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

Ah interesting. We can solve both problems by replacing the rm with emptying the file. So the file never disappears, but it still behaves as before and is zeroed out?

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 still don't see a compelling reason to reset the file.

i18n (1.8.10)
i18n (1.8.11)
Copy link
Member

Choose a reason for hiding this comment

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

❗ It looks like you probably ran bundle update xdrgen and it has updated another dependency that is unnecessary. Ruby's bundler will update any dependency it can. Can you change this back to 1.8.10? I think we can avoid updating other dependencies unless we see a need.

Copy link
Contributor Author

@2opremio 2opremio Nov 10, 2021

Choose a reason for hiding this comment

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

I ran a conservative update, only on xdrgen.

Copy link
Member

Choose a reason for hiding this comment

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

Even on conservative updates bundler will update all transitive dependencies for the gem you requested be updated.

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 don't see it as a very important side effect.

@leighmcculloch leighmcculloch changed the title Add stellar/xdrgen#68 xdr: update using stellar/xdrgen#68 Nov 10, 2021
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

LGTM.

@bartekn
Copy link
Contributor

bartekn commented Nov 10, 2021

I was curious and run CPU profiler on Horizon 2.10.0 and master on a small set of latest pubnet ledgers. Highlighted all xdr.* calls. TransactionProcessor is now faster then OperationProcessor on my machine! Great job @leighmcculloch and @2opremio!

horizon-v2.10.0
Screenshot 2021-11-10 at 23 46 11

master
Screenshot 2021-11-10 at 23 46 21

@2opremio
Copy link
Contributor Author

@bartekn is there a benchmark for that? If so, we should run it when optimizing Horizon. If not, I think we should create one.

@2opremio
Copy link
Contributor Author

I think we could have a benchmark for each transaction processor and one for ingestion in general.

@bartekn
Copy link
Contributor

bartekn commented Nov 11, 2021

I think benchmark makes sense but only with mocked DB to filter it out. I think we can do (in a separate PR) but we are constantly monitoring Horizon metrics so not sure if it's required.

@2opremio 2opremio merged commit e9d8dbd into stellar:master Nov 11, 2021
@2opremio 2opremio deleted the xdrgen-68 branch November 11, 2021 00:09
@2opremio
Copy link
Contributor Author

It would be really helpful when working on profiling (not just for monitor how Horizon's performance is going). I think it would be useful even when running against a local DB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants