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

small optimisation of the lazy memory model #433

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zapashcanon
Copy link
Member

@zapashcanon zapashcanon commented Sep 4, 2024

This should be an optimization.

I'm not sure if the compiler will properly optimize { data : (Int32.t, Value.int32) Hashtbl.t option ref. It could remove one layer of indirection by "merging" the option ref part but I'm not sure about it (we should ask @chambart and @lthls).

The idea is the following:

  • instead of creating an empty Hashtbl, we use an option;
  • we create the table the first time we actually write something;
  • then, when cloning, if the option is None, our children will never read our data, so we use our parent as the parent of the new children (instead of ourself, that is, the grand-parent of the children will actually be its parent)

When traversing the list of parents, this allows to switch from:

Empty -> NonEmptyA -> NonEmptyB -> Empty -> Empty -> NonEmptyC -> Empty -> NonEmptyD

To:

Empty -> NonEmptyA -> NonEmptyB -> NonEmptyC -> NonEmptyD

The invariant is that only the youngest and the oldest element of the list can be empty. Said otherwise, when looking at the whole tree, only the root node and the leaves may be empty. This should reduce allocation and allow to have less element to traverse and try in the list.

cc @filipeom

(I would like to benchmark this on Test-Comp but also running it in B-Tree would be probably more interesting, but Idk how to run this one..)

@lthls
Copy link

lthls commented Sep 4, 2024

I'm not sure if the compiler will properly optimize { data : (Int32.t, Value.int32) Hashtbl.t option ref. It could remove one layer of indirection by "merging" the option ref part but I'm not sure about it (we should ask @chambart and @lthls).

The compiler can't optimise anything here. Is here any reason to use a ref rather than a mutable field ? Unless the reference is meant to be shared, the mutable field should be much better.
The indirection of the option is unfortunate; Jane Street has a proposal for or_null types, which provide options without indirections and could remove the last extra indirection, but it's not even implemented yet in the flambda-backend repo.

@filipeom
Copy link
Collaborator

filipeom commented Sep 4, 2024

This looks great! I think it will help alot!

(I would like to benchmark this on Test-Comp but also running it in B-Tree would be probably more interesting, but Idk how to run this one..)

It's currently only available here: wasm-se-bench. You can change to_run to either `Owi or `Owi_24 and it should produce a results-\today/results.csv with the execution times. Or, I can just add the B-tree dataset to this repo if you want :)

@zapashcanon
Copy link
Member Author

@lthls, thanks for your answer!

The compiler can't optimise anything here. Is here any reason to use a ref rather than a mutable field ? Unless the reference is meant to be shared, the mutable field should be much better.

It is not meant to be shared for now, but this is probably going to be required for #434. I'll switch to a mutable field for the time being. Thanks for your detailed answer! I have one more question, do you think it could be possible to implement an efficient "nullable ref" type as @¢hambart did in nullable array? Or is there something fundamentally different between references and arrays that would prevent this?

It's currently only available here: wasm-se-bench. You can change to_run to either Owi or Owi_24 and it should produce a results-\today/results.csv with the execution times. Or, I can just add the B-tree dataset to this repo if you want :)

What do you think about doing something like test-comp ? I.e. having the all Wasm code in a git submodule and just keep the OCaml code needed to run the benchmarks. I believe most of the OCaml code can be shared and it's nice to get an unified way to run benchmarks. I'll have a look at your runner.ml code and see what I can do. :) In the mean time, I'm going to run TestComp on this PR.

@lthls
Copy link

lthls commented Sep 5, 2024

do you think it could be possible to implement an efficient "nullable ref" type as @¢hambart did in nullable array? Or is there something fundamentally different between references and arrays that would prevent this?

You could literally implement it as a nullable array of size 1. You can also write a specialised version with a simple mutable record, but if you want the same guarantees as @chambart's arrays (compatibility with Marshalling) you will likely end up with something that is very similar to a nullable array of size 1.

@zapashcanon
Copy link
Member Author

Thanks. We don't care about Marshalling compatibility, I'll try to implement it as a record then.

@filipeom
Copy link
Collaborator

filipeom commented Sep 5, 2024

What do you think about doing something like test-comp ? I.e. having the all Wasm code in a git submodule and just keep the OCaml code needed to run the benchmarks. I believe most of the OCaml code can be shared and it's nice to get an unified way to run benchmarks.

I initially put the tools' comparative evaluation in the wasm-se-bench repo so that you wouldn’t have to deal with my spaghetti code 😅 However, I’ve properly separated the btree dataset into its own repo. I’ll create a PR to add it to the bench directory.

I'll have a look at your runner.ml code and see what I can do. :) In the mean time, I'm going to run TestComp on this PR.

We can probably just reuse the current runner with a few adjustments since I based my runner on yours 😅

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

Successfully merging this pull request may close these issues.

3 participants