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.

Sign up for GitHub

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

Jump to bottom

refactor: use Stack class for control stack in parser #341

Merged
merged 2 commits into from
May 26, 2020
Merged

refactor: use Stack class for control stack in parser #341

merged 2 commits into from
May 26, 2020

Conversation

gumb0
Copy link
Collaborator

@gumb0 gumb0 commented May 26, 2020

Required for #312

axic
axic reviewed May 26, 2020
View reviewed changes
lib/fizzy/stack.hpp
@@ -28,6 +28,12 @@ class Stack : public std::vector<T>

void push(T val) { emplace_back(val); }

template <typename... Args>
void emplace(Args&&... args)
Copy link
Member

Choose a reason for hiding this comment

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

Should add a test case for this.

axic
axic reviewed May 26, 2020
View reviewed changes
lib/fizzy/parser_expr.cpp
@@ -120,7 +121,7 @@ parser_result<Code> parse_expr(const uint8_t* pos, const uint8_t* end, const Mod
// The stack of control frames allowing to distinguish between block/if/else and label
// instructions as defined in Wasm Validation Algorithm.
// For a block/if/else instruction the value is the block/if/else's immediate offset.
std::stack<ControlFrame> control_stack;
Stack<ControlFrame> control_stack;
Copy link
Member

Choose a reason for hiding this comment

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

Does this makes any measurable difference in speed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This uses std::vector instead of std::deque now, but it seems make no difference (see below).

@gumb0 gumb0 force-pushed the change-parser-control-stack branch 2 times, most recently from 2ef08a5 to ebd35f4 Compare May 26, 2020 14:53
@gumb0 gumb0 marked this pull request as ready for review May 26, 2020 15:06
@chfast
Copy link
Collaborator

chfast commented May 26, 2020

Benchmarked this on rather noisy could instance.

fizzy/parse/blake2b_mean                             +0.0342         +0.0341            17            17            17            17
fizzy/parse/ecpairing_mean                           -0.0104         -0.0104           932           922           932           922
fizzy/parse/keccak256_mean                           -0.0069         -0.0069            27            27            27            27
fizzy/parse/memset_mean                              -0.0159         -0.0160             4             4             4             4
fizzy/parse/mul256_opt0_mean                         -0.0166         -0.0166             5             5             5             5
fizzy/parse/sha1_mean                                +0.0078         +0.0078            26            26            26            26
fizzy/parse/sha256_mean                              +0.0364         +0.0364            41            43            41            43
fizzy/parse/micro/factorial_mean                     -0.0099         -0.0100             1             1             1             1
fizzy/parse/micro/fibonacci_mean                     -0.0123         -0.0123             1             1             1             1
fizzy/parse/micro/host_adler32_mean                  -0.0132         -0.0132             2             2             2             2
fizzy/parse/micro/spinner_mean                       +0.0185         +0.0184             1             1             1             1

@gumb0 gumb0 force-pushed the change-parser-control-stack branch from ebd35f4 to c8bc582 Compare May 26, 2020 17:17
@gumb0 gumb0 merged commit 7b6dafa into master May 26, 2020
@gumb0 gumb0 deleted the change-parser-control-stack branch May 26, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast approved these changes

@axic axic axic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gumb0 @chfast @axic