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

Simc WebAssembly #4341

Open
athei opened this issue Jun 29, 2018 · 15 comments
Open

Simc WebAssembly #4341

athei opened this issue Jun 29, 2018 · 15 comments

Comments

@athei
Copy link
Contributor

athei commented Jun 29, 2018

I am currently experimenting with creating a web assembly module from the simulationcraft codebase by compiling it with emscripten. In the long run I want something that can be used to start a side like raidbots.com but without the need to have some costly server to run the simulations on. I also do not want the users to have some client to install and connect it to a side. Running simulationcraft as part of the side is the best of both worlds. What I basically want to create is an easy way to compile simulationcraft to an wasm file that can be included into a website and called into from javascript like this:

var simc = Simc()
simc.simulate("profile")

I managed to come up with some proof of concept where I have a simc running inside the browser and doing successful simulations. There is still a lot of work to do. I am interested in mainstreaming it.

Would that be something you are interested in merging even though that would mean to merge some larger patches that touch multiple and restructure the build system?

The largest part that need to change the build system that needs to be more flexible and needs to allow for enabling and disabling features. This is necessary because not all parts of simc are useful for the wasm target and we need a way to easily switch them on and off. Right now I removed all dependencies on phtreads because the threading support in the browser is highly unstable and only possible with Firefox Nightly.

I think the easiest thing would be to apply the needed changes only to the cmake system and make it to a first class citizen.
Therefore I would start with merging some patches where I add some missing functionality to the cmake scripts and and add some more #defines that are switchable through cmake options. For example:

  • SC_MULTITHREADING: Threading support is not mature in browsers. Should be disabled for the wasm target until browsers get better.

Other things can be removed too for wasm because we need fewer things and want to shrink the size of the binary since it must be transferred via network on page load.

I also would also remove the file globbing from the cmake script and replace it with explicitly defined *.cpp files to compile. In the process I might change the directory structure a bit. The reason is that I need to select different files based on selected options and wether I compile for wasm or for your computer. For example I implemented a mock_concurrency.cpp that does basically nothing and removed the sc_profileset.cpp files from the build since they use std::thread directly without the wrapper from concurrency.hpp.

There are some other issues that make this hard. For example I am not sure from which branch I should start my wasm branch. There is the default legion-dev branch and the bfa-dev feature branch. Problem is that non-bfa code changes (like improvements to cmake scripts) are merged to the feature branch instead of to the main branch.

Will bfa-devand legion-dev be merged into a new master on bfa release and should I wait for that before I start a pull request? And for right now should I branch off bfa-dev to have fewer merge conflicts later on?

@scamille
Copy link
Member

About branches: work on bfa-dev, which has become a fork of legion-dev and is the future for any BFA simc development. There will be no merges from legion-dev to bfa-dev anymore.

I did not understand everything, but from what I grasped, my opinion on things would be as followed: There should not be any wasm things inside simc main repository directly. Whatever you need to actually build the webassembly wrapper should go into your own repository.

On the other side you have a free hand in restructuring simc and it's build system to your likes, as long as normal simc still builds and works as it does now. Of course a bit of coordination and not too much negative impact would be great.

Cmake as a build system is not yet established very much here, so feel free to mess with it.
On the negative for you is that when you want to restructure things you would need the other build systems as well.

@athei
Copy link
Contributor Author

athei commented Jun 29, 2018

About branches: work on bfa-dev, which has become a fork of legion-dev and is the future for any BFA simc development. There will be no merges from legion-dev to bfa-dev anymore.

Good to know. Thanks.

I did not understand everything, but from what I grasped, my opinion on things would be as followed: There should not be any wasm things inside simc main repository directly. Whatever you need to actually build the webassembly wrapper should go into your own repository.

There is little that is to be added that could go in a separate repository. Basically its about adding switches and #ifdefs so we are able to remove certain things that are not compatible with the browser or not needed. That makes sense for the normal simc, too. The "webassembly wrapper" would just be a single sc_wasm.cppthat is compiled instead of sc_main.cpp. All of this would not break the normal simc build and is selected by the build system. I think integrating all of this inside the simc repository is the best way to go.

On the other side you have a free hand in restructuring simc and it's build system to your likes, as long as normal simc still builds and works as it does now. Of course a bit of coordination and not too much negative impact would be great.

I may start with just that without adding any wasm specific things. After this is done adding the sc_wasm.cppand the cmake code is easy.

Cmake as a build system is not yet established very much here, so feel free to mess with it.
On the negative for you is that when you want to restructure things you would need the other build systems as well.

I will make it in a way that the old make file still works but add no new features to it. What this basically means is that the old make file always build with all features just as it does now. So I can get away with none to few changes to the Makefile. I can do this by formulating the options in a negative way. SC_NOTHREADS. The makefile never supplied this flags because it does not know it and therefore continues working.

@scamille
Copy link
Member

If you really only have a single file and this is properly maintained and makes sense directly in the main repository then that is fine. We just don't want any potential maintenance burden and extra dependencies/stuff which might become obsolete.

I did not mean that any of those new selection-features must go into any of the other builds files. Only if you want to restructure any file structure. There are also 2 vs solutions and 1 qmake ;)

@navv1234 what is your opinion on this?

@athei
Copy link
Contributor Author

athei commented Jun 29, 2018

We just don't want any potential maintenance burden and extra dependencies/stuff which might become obsolete.

I can totally understand that. That is really a single file that does nothing more than exporting a C compatible interface to some engine functionality.

The other thing that need to be added is some code to the cmake that selects the correct C++ to wasm compiler and do some things in a different way.

I am totally fine with having these two things in a separate repository of mine. I just think that it makes the build process awkward because I have to either replicate the build system in my repository and keep it in sync or have to design the cmake in the simc repository in a way that allows me too hook deeply into it which makes it more complex.

But in the end it is you who need to maintain it and therefore it is absolutely your right to refuse this proposal. I can only say that I intent to be a user of this feature and therefore have an interest in maintaining it. If not you can certainly remove it if it gets obsolete.

I did not mean that any of those new selection-features must go into any of the other builds files. Only if you want to restructure any file structure. There are also 2 vs solutions and 1 qmake ;)

I will try to avoid those changes in order to keep modifications to the other build systems to a minimum.

@scamille
Copy link
Member

Btw: If you want you can hook into source_files/synchronize.py to generate the source file list for cmake.

Binary size of simc can be quite large, since we statically link in a lot of spell and in particular item data. If there is any possibility of reducing size there you would need to discuss it with navv. You can also find us on Discord, see Readme.

@athei
Copy link
Contributor Author

athei commented Jun 29, 2018

Binary size of simc can be quite large, since we statically link in a lot of spell and in particular item data. If there is any possibility of reducing size there you would need to discuss it with navv. You can also find us on Discord, see Readme.

I can just reduce the size by deselecting features. But generally I do not think that binary size is too large right now. A fully optimized and gzipped wasm module with alle features enabled is around 7MB. The item data compresses nicely.

@navv1234
Copy link
Contributor

I have no issues whatsoever with this, either through pull requests or I can give you commit access. I would hope that the interfacing with emscripten/wasm stuff can be done in a way that is fully isolated (behind some flag) on the CMake side, in which case the maintenance burden of that part is on the people involved on that side of things.

Another thing to note is that it's probably semi unreasonable to demand people to take into account the wasm stuff when they develop the modules, so if something on that end does not work, it's probably up to the wasm side of things to fix them. Same goes for core features, I would hope that I don't need to worry about too much when adding new features to the core. If it's just threading-related stuff that I need to worry about, I can handle that fine.

IMO the most important thing is that you won't break existing build systems, which may be slightly annoying seeing as we have so many. Also, as Serge said, you'll probably want to look into incorporating the object file generation into synchronize.py, otherwise things will break from time to time for you if people add new files.

Seems least invasive way to do that is to just add various "disable" flags in the code to the places you need to disable. The conventional build system would just then be none the wiser in these cases and carry on as normal.

@athei
Copy link
Contributor Author

athei commented Jul 1, 2018

I have no issues whatsoever with this, either through pull requests or I can give you commit access.

I am currently cooking up a first pull request that just moves some file arounds and adjusts the build systems and has nothing to do with wasm. Just as a preparation. I will post it soon for discussion. We can talk about commit rights after that to reduce the amount of requests you need to review.

Another thing to note is that it's probably semi unreasonable to demand people to take into account the wasm stuff when they develop the modules, so if something on that end does not work, it's probably up to the wasm side of things to fix them. Same goes for core features, I would hope that I don't need to worry about too much when adding new features to the core. If it's just threading-related stuff that I need to worry about, I can handle that fine.

I think we will be fine here. Basically everything will compile for wasm. It is just the threading thing that must be disabled for now until browsers catch up. The rest is just bonus to slim down the binary but would not brake wasm build. For day-to-day commits there is basically nothing that would brake wasm. Just build system changes or threading things.

IMO the most important thing is that you won't break existing build systems, which may be slightly annoying seeing as we have so many. Also, as Serge said, you'll probably want to look into incorporating the object file generation into synchronize.py, otherwise things will break from time to time for you if people add new files.

I work hard to not brake them. I looked into synchronize.py and noticed that it does overwrite CMakeLists.txt files. I do not think that this is a good idea because most of the code there will be handwritten and the source files will be determined by just globbing. In my pull request I post soon I moved all the engine files to engine/lib and the simc main to engine/simc/sc_main.cpp to make it really easy to just use globbing from cmake. You can comment on that in the pull request I post soon and also check that I did not brake any build system in the process.

@navv1234
Copy link
Contributor

navv1234 commented Jul 1, 2018

Why do we need to restructure the whole project file-wise to support this? I thought you intended to move away from the globbing in the first place?

@athei
Copy link
Contributor Author

athei commented Jul 1, 2018

That was before I knew about the existence of synchronize.py. I thought that globbing was the only thing that was viable at the moment because you did not want to maintain a list of files. It was a way to make globbing better.

With the new knowledge I could integrate cmake into synchronize.py without overwriting files just like for the other build systems and completely move away from globbing. Then a directory restructure is not needed. Is that more appropriate?

@navv1234
Copy link
Contributor

navv1234 commented Jul 1, 2018

That sounds more sensible, and avoids having issues down the line when/if new files are added anyhow.

@athei
Copy link
Contributor Author

athei commented Jul 2, 2018

That sounds more sensible, and avoids having issues down the line when/if new files are added anyhow.

I agree. Just opened a pull request doing in that way.

@scamille scamille changed the title Porting simc to the browser Simc WebAssembly Jul 20, 2018
@athei
Copy link
Contributor Author

athei commented Jul 24, 2018

I am currently working on the wasm version in my simc fork on the wasm branch. I developing it in tandem with a web UI (written with react) to explore the interface simc needs to expose. Also some minor changes must be made to make the output more flexible (output json to buffer instead of file for example). The web UI source will be published in a github repository when I release a first site using the UI.

I will take some time before I make an effort to to upstream the changes because the requirements change while developing the web UI. Currently it is not a problem to develop out-of-tree because the current changes that are applied upstream are not touching core parts of simc.

Maybe it could be helpful to upstream a basic version to prevent merge conflicts in the future, which will happen when changes are made to the cmake upstream.

@scamille
Copy link
Member

Thanks for the update.

So how exactly are your plans to get the JSON output? An option to disable normal stdout + text report stdout, and write json report to stdout?
If there is anything we can do to help with simc API, which in general be a benefit for simc, feel free to specify what would be required and maybe we can help you to develop something.

@athei
Copy link
Contributor Author

athei commented Jul 24, 2018

So how exactly are your plans to get the JSON output?

I do not have a concrete plan, yet. Currently I have a workaround that uses files.

An option to disable normal stdout + text report stdout, and write json report to stdout?

Write json to a buffer. For example a std::stringstream.

If there is anything we can do to help with simc API, which in general be a benefit for simc, feel free to specify what would be required and maybe we can help you to develop something.

That would be so nice of you :). I specified what I need in #4383.

@athei athei removed their assignment Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants