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

Upgrade wgpu version to "0.5.0" and add server-side code for id recycling for WebGPU #26564

Merged
merged 2 commits into from May 21, 2020

Conversation

@kunalmohan
Copy link
Collaborator

kunalmohan commented May 18, 2020

I have updated the cargo.lock to use a wgpu-core at a more recent commit where IdentityHandlerFactory was introduced.
r?@kvark


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented May 18, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/identityhub.rs
  • @KiChjang: components/script/dom/identityhub.rs, components/script_traits/script_msg.rs
@highfive
Copy link

highfive commented May 18, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 18, 2020

@kvark could you take a look at this before I proceed further?
Version update isn't complete yet. I'll open a separate PR first to update the existing implementation to the required commit version of wgpu-core and rebase this.

Copy link
Member

kvark left a comment

Thank you, this is one of the critical things we were missing!
Expressed some ideas on how we can rework this to be even better :)

components/script/dom/identityhub.rs Outdated Show resolved Hide resolved
components/webgpu/Cargo.toml Outdated Show resolved Hide resolved
components/webgpu/identity.rs Outdated Show resolved Hide resolved
@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-id-rotation branch from 1fccc9c to ab0aa1a May 20, 2020
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 20, 2020

It was difficult to upgrade to 0.5 without implementing the IdentityRecyclerFactory, so I have completed the task here itself. This PR now covers the following-

  1. Upgrade to version wgpu-core 0.5
  2. Since the spec doesn't include MapReadAsync anymore, I have removed it completely (instead of updating it with the version).
  3. Implement the server-side part for id recycling.
@kunalmohan kunalmohan changed the title Add server-side code for id recycling for WebGPU Upgrade wgpu version to "0.5.0" and add server-side code for id recycling for WebGPU May 20, 2020
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 20, 2020

The upgrade has introduced duplicate dependencies-

./Cargo.lock:1: duplicate versions for package `ron`
	The following packages depend on version 0.1.7 from 'crates.io':
	The following packages depend on version 0.5.1 from 'crates.io':
./Cargo.lock:1: duplicate versions for package `parking_lot_core`
	The following packages depend on version 0.6.2 from 'crates.io':
	The following packages depend on version 0.7.2 from 'crates.io':
./Cargo.lock:1: duplicate versions for package `metal`
	The following packages depend on version 0.17.0 from 'crates.io':
	The following packages depend on version 0.18.0 from 'crates.io':
./Cargo.lock:1: duplicate versions for package `parking_lot`
	The following packages depend on version 0.10.2 from 'crates.io':
	The following packages depend on version 0.9.0 from 'crates.io':

I tried resolving them with ./mach cargo-update .. but wasn't successful.

@kvark
Copy link
Member

kvark commented May 20, 2020

The upgrade has introduced duplicate dependencies

Then we need a separate PR to Servo that bumps parking_lot, another one that bumps RON.

@jdm
Copy link
Member

jdm commented May 20, 2020

You can add duplicates to servo-tidy.toml if it's not straightforward to update the packages that depend on the older versions.

@jdm
Copy link
Member

jdm commented May 20, 2020

And yes, the output from tidy in that case is less helpful than it could be: #25508

@kvark
kvark approved these changes May 20, 2020
Copy link
Member

kvark left a comment

I think it looks good now. Have a few more minor suggestions.
The main unresolved thing is duplicated dependencies. If @jdm is fine to follow up with them, so be it:)

components/webgpu/identity.rs Outdated Show resolved Hide resolved
components/webgpu/identity.rs Show resolved Hide resolved
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 20, 2020

@jdm parking_lot is a dependency of many packages, one of which is Surfman. metal also is a dependency for Surfman. ron is required by Webrender. I guess upgrading them wouldn't be possible from here. So I'll proceed with the servo-tidy.toml option. Or should I open PRs in respective repositories?

@kunalmohan kunalmohan added this to In progress in WebGPU MVP via automation May 20, 2020
@jdm
Copy link
Member

jdm commented May 20, 2020

opening pull requests can't hurt!

@jdm
Copy link
Member

jdm commented May 20, 2020

but yes, no need to gate this PR on those other repositories.

@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-id-rotation branch from 9cc3ee1 to 5d027d6 May 21, 2020
@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 21, 2020

I have made the required changes and squashed the commits.

@kunalmohan kunalmohan marked this pull request as ready for review May 21, 2020
components/webgpu/identity.rs Outdated Show resolved Hide resolved
…r wgpu id recycling

Remove current implementation of MapReadAsync
@kunalmohan kunalmohan force-pushed the kunalmohan:gpu-id-rotation branch from 5d027d6 to a4f9116 May 21, 2020
@kvark
kvark approved these changes May 21, 2020
@jdm
Copy link
Member

jdm commented May 21, 2020

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2020

📌 Commit a4f9116 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2020

Testing commit a4f9116 with merge 137803c...

bors-servo added a commit that referenced this pull request May 21, 2020
Upgrade wgpu version to "0.5.0" and add server-side code for id recycling for WebGPU

<!-- Please describe your changes on the following line: -->
I have updated the cargo.lock to use a wgpu-core at a more recent commit where IdentityHandlerFactory was introduced.
r?@kvark

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented May 21, 2020

thread 'rustc' panicked at 'could not find markdown in source', src/librustdoc/passes/mod.rs:411:31
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1069
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1504
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:198
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:218
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:511
  11: rust_begin_unwind
             at src/libstd/panicking.rs:419
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:111
  13: core::option::expect_failed
             at src/libcore/option.rs:1260
  14: rustdoc::passes::source_span_for_markdown_range
  15: <rustdoc::passes::check_code_block_syntax::SyntaxChecker as rustdoc::fold::DocFolder>::fold_item
  16: <alloc::vec::Vec<T> as alloc::vec::SpecExtend<T,I>>::from_iter
  17: <core::iter::adapters::Map<I,F> as core::iter::traits::iterator::Iterator>::fold
  18: <std::collections::hash::map::HashMap<K,V,S> as core::iter::traits::collect::FromIterator<(K, V)>>::from_iter
  19: rustdoc::passes::check_code_block_syntax::check_code_block_syntax
  20: rustc_middle::ty::context::tls::enter_global
  21: rustc_interface::interface::run_compiler_in_existing_thread_pool
  22: rustdoc::core::run_core
  23: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  24: rustc_driver::catch_fatal_errors
error: Could not document `gfx-backend-empty`.
@kvark
Copy link
Member

kvark commented May 21, 2020

This is rust-lang/rust#70874, which was fixed. Sounds like we need a rustc version update in Servo before moving on?

@jdm
Copy link
Member

jdm commented May 21, 2020

@kunalmohan You should be able to change the nightly version in the rust-toolchain file to any nightly that is older than 5/19/2020.

@kunalmohan
Copy link
Collaborator Author

kunalmohan commented May 21, 2020

I was getting this warning at a few places after toolchain update-
unused return value of `std::mem::replace` that must be used
So I have prefixed the statements with let _ =

@jdm
Copy link
Member

jdm commented May 21, 2020

@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2020

📌 Commit f3fa53b has been approved by jdm

@highfive highfive assigned jdm and unassigned kvark May 21, 2020
@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2020

Testing commit f3fa53b with merge 94063d6...

@bors-servo
Copy link
Contributor

bors-servo commented May 21, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 94063d6 to master...

@bors-servo bors-servo merged commit 94063d6 into servo:master May 21, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
WebGPU MVP automation moved this from In progress to Done May 21, 2020
@kunalmohan kunalmohan deleted the kunalmohan:gpu-id-rotation branch May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
WebGPU MVP
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.