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

feat: enable wasm compilation #2113

Merged
merged 32 commits into from
Jun 25, 2024
Merged

feat: enable wasm compilation #2113

merged 32 commits into from
Jun 25, 2024

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Jun 20, 2024

This updates and replaces #2064 - I suggest you have a look before looking into this PR.

With the changes added in this PR, we can support webassembly compilation without adding an explicit dependency on package:web and thus breaking compatibility with older dart/flutter versions.

As such, this should not be breaking for anyone. I've added tests for every minor dart version since 2.17 2.18 and everything seems to be in order.

There's one caveat: we're losing some points in the new pana check that downgrades dependencies fully berofe running dart analyze. The weird thing is, it's supposed to be reproducible with doing dart pub downgrade followed by dart analyze, but that doesn't work (i.e. it won't fail the same way pana does - actually it doesn't show any issues that way).

In any case, with compilation working fine for all dart versions, I think it's safe to ignore the 20 point reduction for now (until we drop old Flutter version support and can add package:web dependency directly).

closes #2064
closes #2110

Copy link
Contributor

github-actions bot commented Jun 20, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 3216f42

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.64%. Comparing base (389a4e1) to head (3216f42).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2113   +/-   ##
=======================================
  Coverage   88.64%   88.64%           
=======================================
  Files         223      223           
  Lines        7583     7583           
=======================================
  Hits         6722     6722           
  Misses        861      861           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

github-actions bot commented Jun 20, 2024

Android Performance metrics 🚀

  Plain With Sentry Diff
Startup time 384.63 ms 483.96 ms 99.33 ms
Size 6.35 MiB 7.34 MiB 1008.33 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
0f067d3 359.56 ms 431.28 ms 71.72 ms
4b943a1 348.17 ms 437.15 ms 88.98 ms
5aba417 355.78 ms 450.39 ms 94.61 ms
b0811cc 580.00 ms 675.69 ms 95.69 ms
04db237 330.16 ms 428.38 ms 98.22 ms
c732386 316.84 ms 390.62 ms 73.78 ms
74e1fdd 398.61 ms 418.43 ms 19.82 ms
453e1bc 320.41 ms 372.73 ms 52.32 ms
051e97a 359.56 ms 426.68 ms 67.12 ms
6fedcab 388.26 ms 487.42 ms 99.16 ms

App size

Revision Plain With Sentry Diff
0f067d3 6.33 MiB 7.30 MiB 992.08 KiB
4b943a1 6.34 MiB 7.28 MiB 968.41 KiB
5aba417 5.94 MiB 6.96 MiB 1.02 MiB
b0811cc 6.33 MiB 7.27 MiB 954.02 KiB
04db237 5.94 MiB 6.95 MiB 1.01 MiB
c732386 6.26 MiB 7.20 MiB 958.78 KiB
74e1fdd 6.33 MiB 7.27 MiB 954.12 KiB
453e1bc 5.94 MiB 6.95 MiB 1.01 MiB
051e97a 6.27 MiB 7.20 MiB 959.09 KiB
6fedcab 6.33 MiB 7.30 MiB 987.83 KiB

@ueman
Copy link
Collaborator

ueman commented Jun 20, 2024

fix: changes min flutter version to 3.13.0 and dart sdk 3.1.0 to be compatible with package:web 0.5.1

That should allow you to get rid of various backwards compatibility code in the Flutter SDK

@vaind
Copy link
Collaborator Author

vaind commented Jun 20, 2024

fix: changes min flutter version to 3.13.0 and dart sdk 3.1.0 to be compatible with package:web 0.5.1

That should allow you to get rid of various backwards compatibility code in the Flutter SDK

we're actually not going to do that now. the message was autogenerated from the commits but I've rolled back the change. More history in #2064

@vaind
Copy link
Collaborator Author

vaind commented Jun 20, 2024

For future reference, Dart 2.17 tests fail after a couple of tests with

===== CRASH =====
si_signo=Segmentation fault(11), si_code=1, si_addr=0x18
version=2.17.7 (stable) (Mon Aug 22 10:43:32 2022 +0200) on "linux_x64"
pid=1792, thread=1825, isolate_group=main(0x5571f6910800), isolate=test/sentry_measurement_unit_test.dart(0x5571f84e8000)
isolate_instructions=5571f450a460, vm_instructions=5571f450a460
  pc 0x00005571f4bb7c4e fp 0x00007f09780fd150 dart::kernel::ScopeBuilder::LookupVariable(long)+0x1fe
  pc 0x00005571f4bb7a0f fp 0x00007f09780fd170 dart::kernel::ScopeBuilder::VisitVariableGet(long)+0xf
  pc 0x00005571f4bb5960 fp 0x00007f09780fd1b0 dart::kernel::ScopeBuilder::VisitExpression()+0x610
  pc 0x00005571f4bb62b8 fp 0x00007f09780fd280 dart::kernel::ScopeBuilder::VisitConstructor()+0x228
  pc 0x00005571f4bb439d fp 0x00007f09780fd400 dart::kernel::ScopeBuilder::BuildScopes()+0x97d
  pc 0x00005571f476c0e7 fp 0x00007f09780fd740 dart::ParsedFunction::EnsureKernelScopes()+0x37
  pc 0x00005571f4b7dcc3 fp 0x00007f09780fd770 dart::kernel::StreamingFlowGraphBuilder::ParseKernelASTFunction()+0x53
  pc 0x00005571f4b7db21 fp 0x00007f09780fd820 dart::kernel::StreamingFlowGraphBuilder::BuildGraph()+0xb1
  pc 0x00005571f4b916d5 fp 0x00007f09780fdae0 dart::kernel::FlowGraphBuilder::BuildGraph()+0x65
  pc 0x00005571f48070f0 fp 0x00007f09780fdd20 dart::DartCompilationPipeline::BuildFlowGraph(dart::Zone*, dart::ParsedFunction*, dart::ZoneGrowableArray<dart::ICData const*>*, long, bool)+0x40
  pc 0x00005571f4808112 fp 0x00007f09780fe430 dart::CompileParsedFunctionHelper::Compile(dart::CompilationPipeline*)+0x4d2
  pc 0x00005571f4808d8c fp 0x00007f09780fe640 dart+0x21e3d8c
  pc 0x00005571f4809175 fp 0x00007f09780fe680 dart::Compiler::EnsureUnoptimizedCode(dart::Thread*, dart::Function const&)+0xb5
  pc 0x00005571f47e2a64 fp 0x00007f09780fe6d0 dart::SourceReport::VisitFunction(dart::JSONArray*, dart::Function const&)+0x134
  pc 0x00005571f47e2efe fp 0x00007f09780fe760 dart::SourceReport::VisitLibrary(dart::JSONArray*, dart::Library const&)+0x12e
  pc 0x00005571f47e338f fp 0x00007f09780fe800 dart::SourceReport::PrintJSON(dart::JSONStream*, dart::Script const&, dart::TokenPosition, dart::TokenPosition)+0x12f
  pc 0x00005571f47d355f fp 0x00007f09780fe950 dart+0x21ae55f
  pc 0x00005571f47ca2a1 fp 0x00007f09780feaa0 dart::Service::InvokeMethod(dart::Isolate*, dart::Array const&, bool)+0x321
  pc 0x00005571f47ca9f1 fp 0x00007f09780feac0 dart::Service::HandleIsolateMessage(dart::Isolate*, dart::Array const&)+0x11
  pc 0x00005571f46ab5e8 fp 0x00007f09780feba0 dart::IsolateMessageHandler::HandleMessage(std::__2::unique_ptr<dart::Message, std::__2::default_delete<dart::Message> >)+0x278
  pc 0x00005571f46d454d fp 0x00007f09780fec10 dart::MessageHandler::HandleMessages(dart::MonitorLocker*, bool, bool)+0x14d
  pc 0x00005571f46d4c2f fp 0x00007f09780fec60 dart::MessageHandler::TaskCallback()+0x1df
  pc 0x00005571f47f60c8 fp 0x00007f09780fece0 dart::ThreadPool::WorkerLoop(dart::ThreadPool::Worker*)+0x148
  pc 0x00005571f47f651d fp 0x00007f09780fed10 dart::ThreadPool::Worker::Main(unsigned long)+0x6d
  pc 0x00005571f476b368 fp 0x00007f09780fedd0 dart+0x[214](https://github.com/getsentry/sentry-dart/actions/runs/9600075094/job/26475396946?pr=2113#step:3:253)6368
-- End of DumpStackTrace
=== Crash occured when compiling package:sentry/src/sentry_measurement_unit.dart_InformationSentryMeasurementUnit_InformationSentryMeasurementUnit. in unoptimized JIT mode in unknown pass
=== Flow Graph not available
/home/runner/work/_temp/4e36ea00-b559-4259-86cf-09b27b4f401e.sh: line 1:  1792 Aborted                 (core dumped) dart test -p vm --coverage=coverage --test-randomize-ordering-seed=random --chain-stack-traces

@denrase
Copy link
Collaborator

denrase commented Jun 24, 2024

Just tested this locally with flutter build web --wasm and our example app (commenting out code that does not work with wasm) . Running the app using dart pub global run dhttpd '--headers=Cross-Origin-Embedder-Policy=credentialless;Cross-Origin-Opener-Policy=same-origin' and checking the issues on sentry.io, everything works as expected. 🥳

Didn't even have to add the web package in this test, as we are already adding it implicitly through the http dependency.

Bildschirmfoto 2024-06-24 um 10 42 49

As far as I can tell, this is the best approach, as you have described and for the reasons outlined here.

\cc @buenaflor

Copy link
Collaborator

@denrase denrase left a comment

Choose a reason for hiding this comment

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

Nice, just some questions

@kevmoo
Copy link

kevmoo commented Jun 24, 2024

Let us know if we can help here! This is blocking a critical customer! 😄

Copy link
Contributor

@buenaflor buenaflor left a comment

Choose a reason for hiding this comment

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

nice 🚀

didn't see anything major.

Once all questions are resolved we can go ahead with this 👍

I think the pana thing is fine for now.

Copy link
Contributor

iOS Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.31 ms 1247.23 ms 13.92 ms
Size 8.33 MiB 9.61 MiB 1.27 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
04bd9e6 1230.78 ms 1250.71 ms 19.94 ms
25161f4 1252.35 ms 1263.84 ms 11.49 ms
4656f10 1247.04 ms 1276.16 ms 29.12 ms
abcdba3 1257.31 ms 1283.49 ms 26.18 ms
2966d88 1251.76 ms 1270.21 ms 18.46 ms
333903e 1251.15 ms 1270.21 ms 19.06 ms
2a0edba 1229.27 ms 1259.54 ms 30.27 ms
89ea268 1252.33 ms 1253.58 ms 1.26 ms
50bdfad 1253.14 ms 1274.54 ms 21.40 ms
5aba417 1265.31 ms 1287.90 ms 22.59 ms

App size

Revision Plain With Sentry Diff
04bd9e6 8.33 MiB 9.61 MiB 1.27 MiB
25161f4 8.28 MiB 9.34 MiB 1.06 MiB
4656f10 8.32 MiB 9.50 MiB 1.19 MiB
abcdba3 8.15 MiB 9.12 MiB 989.76 KiB
2966d88 8.32 MiB 9.38 MiB 1.06 MiB
333903e 8.10 MiB 9.16 MiB 1.06 MiB
2a0edba 8.32 MiB 9.52 MiB 1.20 MiB
89ea268 8.09 MiB 9.16 MiB 1.06 MiB
50bdfad 8.32 MiB 9.43 MiB 1.10 MiB
5aba417 8.16 MiB 9.17 MiB 1.01 MiB

Copy link
Collaborator

@denrase denrase left a comment

Choose a reason for hiding this comment

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

Nice, thx for doing this! \o/

@vaind vaind merged commit 7e7f0b1 into main Jun 25, 2024
132 checks passed
@vaind vaind deleted the package-web branch June 25, 2024 08:19
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.

None yet

6 participants