Skip to content

ExpressionRunner class hierarchy #2797

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

Closed
aheejin opened this issue Apr 23, 2020 · 8 comments
Closed

ExpressionRunner class hierarchy #2797

aheejin opened this issue Apr 23, 2020 · 8 comments

Comments

@aheejin
Copy link
Member

aheejin commented Apr 23, 2020

#2702 exposes functionalities of ExpressionRunner to Binaryen and C API. I find most usages there is about precomputing certain expression, and we have PrecomputeExpressionRunner for that. But after #2702 some functionalities of PrecomputeExpressionRunner are duplicated in ExpressionRunner to make them exposed to Binaryen and C API, and I'm not very sure what the difference between ExpressionRunner and PrecomputeExpressionRunner is at this point.

So I'm not sure the functionalities of ExpressionRunner class hierarchy is very well defined now. Can't Binaryen and C API make use of PrecomputeExpressionRunner instead, if the main purpose of that is precomputing?

I might be mistaken, so please advise me if so :)

cc @dcodeIO @kripken @tlively

@dcodeIO
Copy link
Contributor

dcodeIO commented Apr 23, 2020

Yeah, that's a refactor I'm not especially proud of. Essentially, what happened there is that most of PrecomputeExpressionRunner has been refactored into the base ExpressionRunner, since an earlier attempt to move PrecomputeExpressionRunner to wasm-interpreter.h as StandaloneExpressionRunner (extending ExpressionRunner) turned out to be overly confusing in review. So the suggestion of moving most of it into the base class was made and implemented. There's also the situation now where the base ExpressionRunner has some fields that are never used by the derived RuntimeExpressionRunner, which I mentioned on the original PR as it seems unfortunate.

The underlying concern for extending PrecomputeExpressionRunner from ExpressionRunner then was that it does something special with getValues (what seems to be duplicate code), which are being computed during the precompute pass and CExpressionRunner doesn't need. CExpressionRunner exists because it must be final for some C reasons.

Perhaps we should pick up the idea again to add a class in between, what previously was StandaloneExpressionRunner (ContextualExpressoinRunner? ContextAwareExpressionRunner? LinearExpressionRunner? StaticExpressionRunner?), that only PrecomputeExpressionRunner and CExpressionRunner inherit from, but not RuntimeExpressionRunner?

ExpressionRunner
├ WhateverExpressionRunner
│  ├ PrecomputeExpressionRunner
│  └ CExpressionRunner
├ ConstantExpressionRunner
└ RuntimeExpressionRunner

@aheejin
Copy link
Member Author

aheejin commented Apr 23, 2020

since an earlier attempt to move PrecomputeExpressionRunner to wasm-interpreter.h as StandaloneExpressionRunner (extending ExpressionRunner) turned out to be overly confusing in review.

I didn't review that PR, so could you elaborate on what parts were overly confusing in doing that?

@dcodeIO
Copy link
Contributor

dcodeIO commented Apr 23, 2020

Sure, the pivotal discussion was #2702 (comment), then leading to that change.

@aheejin
Copy link
Member Author

aheejin commented Apr 24, 2020

Sorry, I read the comment and am still at a loss.

  • What does it mean to be the execution to be linear?
  • Why is the comment thread a reason that we can't expose PrecomputeExpressionRunner instead so Binaryen and C API can use it?

@dcodeIO
Copy link
Contributor

dcodeIO commented Apr 24, 2020

What does it mean to be the execution to be linear?

This has implications on the way we track intermediate values of locals (which is one of my additions that precompute didn't have previously). If execution was not be linear, we cannot simply use a map that we write to on local.set, and read on local.get. Somewhat analogous to the runner exclusively operating in execution order.

Why is the comment thread a reason that we can't expose PrecomputeExpressionRunner instead so Binaryen and C API can use it?

See also #2804 (comment) on the other thread. Mostly boils down to the precompute pass having one special mechanism.

@kripken
Copy link
Member

kripken commented Apr 24, 2020

I'd support a refactoring here, I agree this should be cleaner. Hopefully we can find a nice solution!

Sorry I didn't push on this more in reviewing that PR. Part of the reason is I had "tunnel vision" in a large and long-running review of a PR - over time in review I focused less on the big picture and more on specific details. But in a big refactoring like that I should have re-read the entire thing more frequently.

@dcodeIO
Copy link
Contributor

dcodeIO commented Apr 24, 2020

Sorry I didn't push on this more in reviewing that PR. Part of the reason is I had "tunnel vision"

Can we please agree that this is all my fault for being terrible at C++ and too inexperienced with Binaryen's codebase, making my PR's hard to review in the first place? :) I promise this will improve a lot over time, when I become more confident! (I guess it also takes a little longer than usual since I'm external and mostly relying on somewhat indirect, so sometimes inefficient means of communication.)

aheejin pushed a commit that referenced this issue Apr 28, 2020
Tackles the concerns raised in #2797 directly related to #2702 by reverting merging all of `PrecomputeExpressionRunner` into the base `ExpressionRunner`, instead adding a common base for both the precompute pass and the new C-API to inherit. No functional changes.

---

### Current hierarchy after #2702 is

```
ExpressionRunner
├ [PrecomputeExpressionRunner]
├ [CExpressionRunner]
├ ConstantExpressionRunner
└ RuntimeExpressionRunner
```

where `ExpressionRunner` contains functionality not utilized by `ConstantExpressionRunner` and `RuntimeExpressionRunner`.

### New hierarchy will be:

```
ExpressionRunner
├ ConstantExpressionRunner
│  ├ [PrecomputeExpressionRunner]
│  └ [CExpressionRunner]
├ InitializerExpressionRunner
└ RuntimeExpressionRunner
```

with the precompute pass's and the C-API's shared functionality now moved out of `ExpressionRunner` into a new `ConstantExpressionRunner`. Also renames the previous `ConstantExpressionRunner` to `InitializerExpressionRunner` to [better represent its uses](https://webassembly.org/docs/modules/#initializer-expression) and to make its previous name usable for the new intermediate template, where it fits perfectly. Also adds a few comments answering some of the questions that came up recently.

### Old hierarchy before #2702 for comparison:

```
ExpressionRunner
├ [PrecomputeExpressionRunner]
├ ConstantExpressionRunner
└ RuntimeExpressionRunner
```
@aheejin
Copy link
Member Author

aheejin commented Apr 28, 2020

Addressed by #2804. Thanks!

@aheejin aheejin closed this as completed Apr 28, 2020
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

No branches or pull requests

3 participants