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

Update the emscripten version to 2.0.23 #681

Merged
merged 26 commits into from
Jun 24, 2021

Conversation

Devika-Tantry
Copy link
Contributor

@Devika-Tantry Devika-Tantry commented Jun 9, 2021

ASW work item : Update emscripten tooling used to build Vireo
Ran ASW PR with this updated package for test.

Changes:

  • Updated emscripten version to 2.0.23 , python to 3.9
  • Error fix in casting case. (It was due to an upgrade in clang it promotes all warnings to error now.)
  • Flag fix as per release note :
    - BINARYEN_TRAP_MODE=js is depreciated.
    - -s NO_EXIT_RUNTIME=1 is by default set to 1 so removed.
    - EXTRA_EXPORTED_RUNTIME_METHODS renamed to EXPORTED_RUNTIME_METHODS
    - Updated the flag in debug build to -g
    image
    so now debug output will have DWARF info.
    Note: With this updated vireo package we can run the web gvi's in Chrome Canary.(with setup mentioned here).
    -g applies on debug build so we have to use debug wasm to directly interact with CPP files.

In WebVI, we use Viero SDK wasm build. Screenshots of putting breakpoints in Vireo SDK CPP files and watching values in the browser in WebVI.
image

Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

@Devika-Tantry some feedback:

  1. The Error: No tool or SDK found by name '2.0.20'. is likely due to the version of the emsdk tools used. We checkout a specific commit (web reference, web-windows reference) because the tools can be unreliable sometimes. Try changing to one of the newer commits from emscripten-core/emsdk. I would recommend the latest emsdk 2.0.23 release commit.
  2. In the PR comment can you link to the associated ASW work item
  3. Can you run an ASW PR build with your local vireo change and make sure to enable the [VireoTest] suite for the build (see the section "Trigger the Web Module Squad tests").
  4. Include a link to the ASW build result run with [VireoTest] in the PR
  5. (optional but it's cool) Sujay shared a screenshot of being able to debug Vireo inside Chrome Canary which is awesome! Can you add a screenshot showing debugging in the browser to the PR description? Shows the benefit of updating the toolchain :)
  6. For the flag changes could you add comments in the PR for those lines that points to the change description? For example it looks like this is a reference to the NO_EXIT_RUNTIME change. Went ahead and added some comments as I was interested in the changes.

@Devika-Tantry Devika-Tantry changed the title Update the emscripten version to 2.0.11 Update the emscripten version to 2.0.23 Jun 9, 2021
@rajsite
Copy link
Member

rajsite commented Jun 9, 2021

Looks like the PR is still in-work but a couple more notes:

  1. Can you add an entry to the .gitignore for the .vs directory so those files are not checked in?
  2. Can we scope the changes as close as possible to just the compiler update? ie lets not update the python and node versions unless it is required

@Devika-Tantry Devika-Tantry marked this pull request as draft June 9, 2021 16:39
Copy link
Contributor

@cglzaguilar cglzaguilar left a comment

Choose a reason for hiding this comment

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

I have not additional questions. Please address the feedback given by @rajsite.

Copy link
Contributor

@sanmut sanmut left a comment

Choose a reason for hiding this comment

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

Thanks. Would like to take another look with feedback addressed.

Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Discussed in a meeting, some outcomes:

  • For WriteDoubleToMemory we should use ConvertFloatToInt instead and use it for all the num types, including 64bit
  • The Double.Test.js should be updated to use the saturating values where relevant
  • We should undo the failed ResizeDimension handling preferring to let the app segfault. We should create a Tech Debt to create an abort mechanism that can abort the runtime quickly closer to the allocation failure (looks like we can call the C abort function).
  • The ExecuteSlicesUntilClumpsFinshed.Test.js error handling test should use a try catch around the execute slices and assert that the error happens. We should create a Tech Debt task to make an instruction that can intentionally fail with a seg fault and update the test to call that instruction.

@Devika-Tantry
Copy link
Contributor Author

Discussed in a meeting, some outcomes:

  • For WriteDoubleToMemory we should use ConvertFloatToInt instead and use it for all the num types, including 64bit
  • The Double.Test.js should be updated to use the saturating values where relevant
  • We should undo the failed ResizeDimension handling preferring to let the app segfault. We should create a Tech Debt to create an abort mechanism that can abort the runtime quickly closer to the allocation failure (looks like we can call the C abort function).
  • The ExecuteSlicesUntilClumpsFinshed.Test.js error handling test should use a try catch around the execute slices and assert that the error happens. We should create a Tech Debt task to make an instruction that can intentionally fail with a seg fault and update the test to call that instruction.

@Devika-Tantry Devika-Tantry requested review from sanmut and rajsite June 21, 2021 18:05
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Looking good, just some minor tweaks remaining

@rajsite rajsite marked this pull request as ready for review June 21, 2021 21:50
@rajsite
Copy link
Member

rajsite commented Jun 21, 2021

@Devika-Tantry We have been creating tech debt items as GitHub issues so far: https://github.com/ni/VireoSDK/issues
I'd prefer to keep Vireo-specific tech debt issues tracked on GitHub if possible so the ideas of how the repo should look or be refactored can be localized to GitHub. Do you know if the squad wants to continue that way?

@Devika-Tantry
Copy link
Contributor Author

@Devika-Tantry We have been creating tech debt items as GitHub issues so far: https://github.com/ni/VireoSDK/issues
I'd prefer to keep Vireo-specific tech debt issues tracked on GitHub if possible so the ideas of how the repo should look or be refactored can be localized to GitHub. Do you know if the squad wants to continue that way?

Created two issues (issue1, issue2) in github to track the issue and added these issue links in corresponding AzDo for squad tracking purpose.

@rajsite
Copy link
Member

rajsite commented Jun 22, 2021

Unfortunately we don't have great performance benchmarks to check for regressions in compiler upgrades that I am aware of. There is the PiCalc test if y'all want to try it.

I did a quick check on size changes and it looks like the wasm file is about 45kb smaller (top is original and bottom is new compiler):
image

Free page size win! :)

@rajsite
Copy link
Member

rajsite commented Jun 22, 2021

@Devika-Tantry minor comment, won't block the PR, but thanks for the work on upgrading the compiler version! Make sure to add yourself to the AUTHORS list :)

@rajsite
Copy link
Member

rajsite commented Jun 23, 2021

@Devika-Tantry Looks like the PR is almost ready 👍
Looking at the ASW PR where you tested changes yesterday one concern I have is that it only seems to have run 8 tests.

We should make sure that the tests run since this is a compiler change. I believe the issue may be that the Run-SquadTests.txt GUID needs to be changed for each build you want to trigger. The most recent commits in that PR do not seem to update the token.

Edit:
My bad, that was the .NET Core build 🤦‍♂️ The ASW PR build ran many more tests. Did you get a chance to evaluate the couple of failures that were in there?

@Devika-Tantry
Copy link
Contributor Author

My bad, that was the .NET Core build 🤦‍♂️ The ASW PR build ran many more tests. Did you get a chance to evaluate the couple of failures that were in there?

These are some timeout error seems like these are common in PR build and these all not related to vireo changes. If we rerun, different set of tests fail due to timeout.

@rajsite rajsite merged commit dd1839e into ni:main Jun 24, 2021
@Devika-Tantry Devika-Tantry deleted the users/devika/UpdateEmscripton branch July 6, 2021 04:18
@Devika-Tantry Devika-Tantry restored the users/devika/UpdateEmscripton branch July 6, 2021 04:18
@Devika-Tantry Devika-Tantry deleted the users/devika/UpdateEmscripton branch July 7, 2021 05:33
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.

5 participants