`worker_id` type changed to uuid #25211

kunalmohan · 2019-12-08T15:33:16Z

Fixes #6631
worker_id is now generate as uuid and saved as string.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #6631 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2019-12-08T15:33:25Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

highfive · 2019-12-08T15:33:26Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/globalscope.rs, components/script/Cargo.toml, components/script/serviceworker_manager.rs
@KiChjang: components/script/dom/globalscope.rs, components/script/Cargo.toml, components/script/serviceworker_manager.rs

highfive · 2019-12-08T15:33:28Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

paulrouget · 2019-12-09T03:38:03Z

components/devtools_traits/lib.rs

-#[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)]
-pub struct WorkerId(pub u32);
+#[derive(Clone, Debug, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)]
+pub struct WorkerId(pub String);


You can keep uuid and implement malloc_size_of, or maybe use u128 (https://docs.rs/uuid/0.8.1/uuid/struct.Uuid.html#method.to_u128_le).

paulrouget · 2019-12-09T03:38:34Z

components/script/dom/globalscope.rs

@@ -94,7 +95,7 @@ impl Drop for AutoCloseWorker {
 pub struct GlobalScope {
    eventtarget: EventTarget,
    crypto: MutNullableDom<Crypto>,
-    next_worker_id: Cell<WorkerId>,
+    next_worker_id: WorkerId,


I don't think that field is necessary anymore. Just create a new uuid when necessary.

kunalmohan · 2019-12-09T08:45:28Z

Ok. I'll start working on these changes.
A small query (and probably a silly one)- malloc_size_of for UUID should return 0, right?

paulrouget · 2019-12-10T05:24:34Z

malloc_size_of for UUID should return 0, right?

I don't know. @jdm ?

jdm · 2019-12-10T05:48:57Z

Yes, that's fine.


        `worker_id` type changed to uuid

Fixes #6631 `worker_id` is now generate as uuid and saved as string.


        `WorkerId` changed to type UUID.


        Undo changes done to change worker_id type to string

kunalmohan · 2019-12-10T07:48:17Z

I have changed worker_id to uuid and implemented malloc_size_of.

components/devtools_traits/Cargo.toml

paulrouget · 2019-12-10T08:41:06Z

components/malloc_size_of/Cargo.toml

@@ -47,3 +47,4 @@ url = { version = "2.0", optional = true }
 webrender_api = { git = "https://github.com/servo/webrender", optional = true }
 xml5ever = { version = "0.16", optional = true }
 void = "1.0.2"
+uuid = {version = "0.8", features = ["v4", "serde"]}


Is "serde" necessary?

I guess I can remove serde here.

Please make uuid optional and enable it as part of the servo feature.

I have removed serde here and made the other changes too.

components/malloc_size_of/lib.rs


        Make uuid optional and hide it behind servo feature in malloc_size_of

jdm · 2019-12-13T14:42:30Z

@bors-servo r+

bors-servo · 2019-12-13T14:42:32Z

📌 Commit a892102 has been approved by jdm

bors-servo · 2019-12-13T15:18:31Z

⌛ Testing commit a892102 with merge dad8784...


        Auto merge of #25211 - kunalmohan:6631-Worker-Id, r=jdm

`worker_id` type changed to uuid  Fixes #6631 `worker_id` is now generate as uuid and saved as string. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #6631 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-12-13T15:50:55Z

💔 Test failed - status-taskcluster

jdm · 2019-12-13T15:52:02Z

@bors-servo retry

#24726

bors-servo · 2019-12-13T16:06:36Z

⌛ Testing commit a892102 with merge d4c6333...


        Auto merge of #25211 - kunalmohan:6631-Worker-Id, r=jdm

`worker_id` type changed to uuid  Fixes #6631 `worker_id` is now generate as uuid and saved as string. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #6631 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

CYBAI · 2019-12-16T14:49:27Z

@bors-servo retry

#24762

bors-servo · 2019-12-16T15:08:24Z

⌛ Testing commit a892102 with merge fca99fd...


        Auto merge of #25211 - kunalmohan:6631-Worker-Id, r=jdm

`worker_id` type changed to uuid  Fixes #6631 `worker_id` is now generate as uuid and saved as string. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #6631 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-12-16T15:19:35Z

💔 Test failed - status-taskcluster

jdm · 2019-12-16T15:39:12Z

@bors-servo retry

#22212

bors-servo · 2019-12-16T16:31:46Z

⌛ Testing commit a892102 with merge c9baa5b...


        Auto merge of #25211 - kunalmohan:6631-Worker-Id, r=jdm

`worker_id` type changed to uuid  Fixes #6631 `worker_id` is now generate as uuid and saved as string. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #6631 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2019-12-16T16:58:01Z

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

highfive assigned paulrouget Dec 8, 2019

highfive added the S-awaiting-review label Dec 8, 2019

paulrouget reviewed Dec 9, 2019

View changes

kunalmohan added 3 commits Dec 8, 2019

`worker_id` type changed to uuid

Verified

This commit was signed with a verified signature.

kunalmohan Kunal Mohan

GPG key ID: 2B475A4524237BAC Learn about signing commits

5c56e66

Fixes #6631 `worker_id` is now generate as uuid and saved as string.

`WorkerId` changed to type UUID.

Verified

This commit was signed with a verified signature.

kunalmohan Kunal Mohan

GPG key ID: 2B475A4524237BAC Learn about signing commits

d5475fb

Undo changes done to change worker_id type to string

Verified

This commit was signed with a verified signature.

kunalmohan Kunal Mohan

GPG key ID: 2B475A4524237BAC Learn about signing commits

Loading status checks…

d60f7d8

paulrouget reviewed Dec 10, 2019

View changes

components/devtools_traits/Cargo.toml Show resolved Hide resolved

paulrouget reviewed Dec 10, 2019

View changes

jdm requested changes Dec 10, 2019

View changes

components/malloc_size_of/lib.rs Show resolved Hide resolved

Make uuid optional and hide it behind servo feature in malloc_size_of

Verified

This commit was signed with a verified signature.

kunalmohan Kunal Mohan

GPG key ID: 2B475A4524237BAC Learn about signing commits

Loading status checks…

a892102

kunalmohan requested a review from jdm Dec 13, 2019

highfive assigned jdm and unassigned paulrouget Dec 13, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 13, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 13, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 13, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 16, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 16, 2019

bors-servo merged commit a892102 into servo:master Dec 16, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 16, 2019

kunalmohan deleted the kunalmohan:6631-Worker-Id branch Jan 2, 2020

servo / servo

`worker_id` type changed to uuid #25211

`worker_id` type changed to uuid #25211

kunalmohan commented Dec 8, 2019

highfive commented Dec 8, 2019

highfive commented Dec 8, 2019

highfive commented Dec 8, 2019

This comment has been minimized.

This comment has been minimized.

kunalmohan commented Dec 9, 2019

paulrouget commented Dec 10, 2019

jdm commented Dec 10, 2019

kunalmohan commented Dec 10, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

jdm commented Dec 13, 2019

bors-servo commented Dec 13, 2019

CYBAI commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

servo / servo

Join GitHub today

`worker_id` type changed to uuid #25211

`worker_id` type changed to uuid #25211

Conversation

kunalmohan commented Dec 8, 2019

highfive commented Dec 8, 2019

highfive commented Dec 8, 2019

highfive commented Dec 8, 2019

This comment has been minimized.

paulrouget Dec 9, 2019

This comment has been minimized.

paulrouget Dec 9, 2019

kunalmohan commented Dec 9, 2019

paulrouget commented Dec 10, 2019

jdm commented Dec 10, 2019

kunalmohan commented Dec 10, 2019

This comment has been minimized.

paulrouget Dec 10, 2019

This comment has been minimized.

kunalmohan Dec 10, 2019

This comment has been minimized.

jdm Dec 10, 2019 • edited

This comment has been minimized.

kunalmohan Dec 11, 2019

jdm commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

jdm commented Dec 13, 2019

bors-servo commented Dec 13, 2019

CYBAI commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm Dec 10, 2019 •

edited