FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX Change rust packages not to build concurrently by ryanking13 · Pull Request #3607 · pyodide/pyodide · GitHub

/ pyodide Public

FIX Change rust packages not to build concurrently - #3607

Merged
ryanking13 merged 9 commits into
pyodide:mainfrom
ryanking13:build-rust
Feb 27, 2023
ConversationCommits (9)ChecksFiles changed
Merged

FIX Change rust packages not to build concurrently#3607
ryanking13 merged 9 commits into
pyodide:mainfrom
ryanking13:build-rust

Conversation

ryanking13 commented Feb 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Resolve #3565

Checklists

hoodmane reviewed Feb 24, 2023
View reviewed changes
Comment thread pyodide-build/pyodide_build/buildall.py Outdated

hoodmane commented Feb 24, 2023

Copy link
Copy Markdown
Member

Also, if you just have two rust packages, this will like spin continuously with the second package? Seems a bit tricky to make it behave reasonably. But maybe it just doesn't matter?

ryanking13 commented Feb 27, 2023

Copy link
Copy Markdown
Member Author

Also, if you just have two rust packages, this will like spin continuously with the second package? Seems a bit tricky to make it behave reasonably. But maybe it just doesn't matter?

Yes, one thread will keep pushing and popping the second package from queue until the first one finishes. I hope it doesn't matter, but when we find it matters, I guess we can add an additional lock for rust packages or incrementally increase the sleep time.

hoodmane commented Feb 27, 2023

Copy link
Copy Markdown
Member

Maybe add a comment indicating that this is what happens when only rust classes are left and we don't exactly love it. Also let's increase the wait time to 50 or 100 ms.

ryanking13 commented Feb 27, 2023

Copy link
Copy Markdown
Member Author

Maybe add a comment indicating that this is what happens when only rust classes are left and we don't exactly love it. Also let's increase the wait time to 50 or 100 ms.

Sounds good. Done.

hoodmane approved these changes Feb 27, 2023
View reviewed changes
Comment thread pyodide-build/pyodide_build/buildall.py Outdated
Apply suggestion [skip ci]
f09306d 
Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>

ryanking13 commented Feb 27, 2023

Copy link
Copy Markdown
Member Author

Thanks for the review!

ryanking13 merged commit c99584d into pyodide:main Feb 27, 2023
ryanking13 deleted the build-rust branch February 27, 2023 05:57
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Mar 2, 2023
FIX Change rust packages not to build concurrently (pyodide#3607)
2cf75e6 
Co-authored-by: Hood Chatham <roberthoodchatham@gmail.com>
ryanking13 mentioned this pull request Mar 13, 2023
Open
hoodmane reviewed Mar 13, 2023
View reviewed changes
Comment thread pyodide-build/pyodide_build/buildall.py
build_queue.put((job_priority(pkg), pkg))

# Release the GIL so new packages get queued
sleep(0.1)

hoodmane Mar 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe we could try releasing the lock before performing this sleep? Since Python threads are somewhere between threads and coroutines and the only yield point is when the lock is held this may have something to do with the problem. Maybe also put one or more unconditional sleep(0) with the lock unacquired.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

hoodmane hoodmane approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issue in building rust-based packages in parallel

2 participants

Back | FazBrowse Home | New Git URL