Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Race condition when saving shaders to disk #14294

Closed
tmpsantos opened this issue Apr 2, 2019 · 4 comments · Fixed by #14707
Closed

[core] Race condition when saving shaders to disk #14294

tmpsantos opened this issue Apr 2, 2019 · 4 comments · Fixed by #14707
Assignees
@tmpsantos
Labels
high priority

Comments

@tmpsantos
Copy link
Contributor

tmpsantos commented Apr 2, 2019 •
edited
Loading

When opening the app for the first time or when after after a GL driver update that breaks the binary shader compatibility, Mapbox GL will re-create the shaders and store them on the filesystem. We have two problems here:

  • If we have multiple maps, there is a race when writing the shaders to disk. Multiple maps might try to write on the same file.
  • There is no recovery from a broken/corrupted shader. File needs to be deleted.
@tmpsantos tmpsantos mentioned this issue Apr 2, 2019
Closed
@tmpsantos tmpsantos self-assigned this Apr 2, 2019
@tmpsantos tmpsantos added the release blocker Blocks the next final release label Apr 2, 2019
tmpsantos added a commit that referenced this issue Apr 2, 2019
@tmpsantos
[core] Disable binary programs until we fix #14294
f38e90a
@tmpsantos
Copy link
Contributor Author

This could buy us time until we find a solution #14298

tobrun pushed a commit that referenced this issue Apr 3, 2019
@tmpsantos @tobrun
[core] Disable binary programs until we fix #14294
b2e416b
@friedbunny friedbunny reopened this Apr 3, 2019
@friedbunny
Copy link
Contributor

Inadvertently closed by the commit message in #14298.

@tmpsantos
Copy link
Contributor Author

Inadvertently closed by the commit message in #14298.

Oh, indeed. Thanks!

tobrun pushed a commit that referenced this issue Apr 8, 2019
@tmpsantos @tobrun
[core] Disable binary programs until we fix #14294
4b63580
tobrun pushed a commit that referenced this issue Apr 12, 2019
@tmpsantos @tobrun
[core] Disable binary programs until we fix #14294
583e332
tobrun pushed a commit that referenced this issue Apr 12, 2019
@tmpsantos @tobrun
[core] Disable binary programs until we fix #14294
7af6b7e
@tobrun tobrun mentioned this issue Apr 18, 2019
Closed
@chloekraw chloekraw removed the release blocker Blocks the next final release label May 10, 2019
@chloekraw
Copy link
Contributor

This is still a release blocker issue if we decide to re-enable binary caching of shaders. Atm there are no plans to do so.

Chaoba pushed a commit that referenced this issue May 15, 2019
@tmpsantos
[core] Disable binary programs until we fix #14294
215da30 
(cherry picked from commit 805b140)
This was referenced May 16, 2019
Closed
Closed
Closed
tobrun pushed a commit that referenced this issue May 16, 2019
@tmpsantos @tobrun
[core] Disable binary programs until we fix #14294
16fe011 
(cherry picked from commit 805b140)
tmpsantos added a commit that referenced this issue May 20, 2019
@tmpsantos
[core] Remove binary shader support
99fbfb0 
The implementation is buggy and not worth maintaining anymore
because performance benefits are not substantial or sometimes
worse. Also, removing it saves about 150 ~ 180 KB in binary size.

Below timings are averages of minimum 5 runs.

```
Device          Init launch     Average relaunch
s10             1129.8 ms       700 ms
s10 - binary    1346.75 ms      694 ms
Pixel           1692 ms         723 ms
Pixel - binary  1883 ms         1039 ms
Kazam           17948 ms        1339 ms
Kazam - binary  19157 ms        1564 ms
Wiko            2060 ms         1278 ms
Wiko - binary   3876 ms         1136 ms
```

Fixes #14294
@tmpsantos tmpsantos mentioned this issue May 20, 2019
Merged
tmpsantos added a commit that referenced this issue May 21, 2019
@tmpsantos
[core] Remove binary shader support
7b8630f 
The implementation is buggy and not worth maintaining anymore
because performance benefits are not substantial or sometimes
worse. Also, removing it saves about 150 ~ 180 KB in binary size.

Below timings are averages of minimum 5 runs.

```
Device          Init launch     Average relaunch
s10             1129.8 ms       700 ms
s10 - binary    1346.75 ms      694 ms
Pixel           1692 ms         723 ms
Pixel - binary  1883 ms         1039 ms
Kazam           17948 ms        1339 ms
Kazam - binary  19157 ms        1564 ms
Wiko            2060 ms         1278 ms
Wiko - binary   3876 ms         1136 ms
```

Fixes #14294
tmpsantos added a commit that referenced this issue May 21, 2019
@tmpsantos
[core] Remove binary shader support
c2f974f 
The implementation is buggy and not worth maintaining anymore
because performance benefits are not substantial or sometimes
worse. Also, removing it saves about 150 ~ 180 KB in binary size.

Below timings are averages of minimum 5 runs.

```
Device          Init launch     Average relaunch
s10             1129.8 ms       700 ms
s10 - binary    1346.75 ms      694 ms
Pixel           1692 ms         723 ms
Pixel - binary  1883 ms         1039 ms
Kazam           17948 ms        1339 ms
Kazam - binary  19157 ms        1564 ms
Wiko            2060 ms         1278 ms
Wiko - binary   3876 ms         1136 ms
```

Fixes #14294
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@tmpsantos tmpsantos

Labels
high priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[core] Remove binary shader support mapbox/mapbox-gl-native
3 participants
@tmpsantos @friedbunny @chloekraw