204871 2019-12-04 20:07:20 -0800 sched_yield can cause calling thread to be preempted for 10 ms on Darwin 2020-08-08 17:30:27 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED DUPLICATE 215248 InRadar P2 Normal --- 1 nham nham benjamin cdumez cmarcelo dbates ews-watchlist fpizlo ggaren nham oldest_to_newest 1595681 0 nham 2019-12-04 20:07:20 -0800 We currently use sched_yield in both bmalloc and WTF::Lock. This can lead to the calling thread being preempted for up to 10 ms even if the lock was relinquished much earlier than that because of the way this is implemented on Darwin: - sched_yield calls the swtch_pri syscall - swtch_pri depresses the calling thread's priority to 0 for thread_depress_time milliseconds - thread_depress_time is set to one scheduling quantum in sched_*.c - most of our devices run with a timer tick of 10 ms, so thread_depress_time is 10 ms - if the system is busy with enough >0 pri work for 10 ms to saturate all possible CPUs, then the calling thread blocks for 10 ms We actually see exactly this in a number of MobileSafari launch tests (this originally came up as a launch test variability issue from the perf team). An alternative is to use thread_switch, which can depress the priority of the calling thread for a configurable amount of time with millisecond granularity. 1595683 1 nham 2019-12-04 20:09:08 -0800 <rdar://problem/57645816> 1595684 2 nham 2019-12-04 20:10:42 -0800 I ran this by Daniel in Core OS (who is one of the scheduler maintainers) and he said that thread_switch(MACH_PORT_NULL, SWITCH_OPTION_DEPRESS, 1) would do basically what sched_yield does, except only depress the priority for 1 ms instead of 10 ms. Of course the kernel folks would rather we use os_unfair_lock rather than rolling our own locks but I think that's probably a separate discussion. 1595689 3 fpizlo 2019-12-04 20:14:59 -0800 (In reply to Ben Nham from comment #2) > I ran this by Daniel in Core OS (who is one of the scheduler maintainers) > and he said that thread_switch(MACH_PORT_NULL, SWITCH_OPTION_DEPRESS, 1) > would do basically what sched_yield does, except only depress the priority > for 1 ms instead of 10 ms. > > Of course the kernel folks would rather we use os_unfair_lock rather than > rolling our own locks but I think that's probably a separate discussion. We use sched_yield because it’s faster than what Daniel suggests. We have tested this many times. We use our own locks because they are faster for WebKit and because they can fit into 2 bits. We use that fact in JSCell. Even WTF::Lock is only one byte, which is 4x smaller than os_unfair_lock. 1595690 4 nham 2019-12-04 20:15:27 -0800 Reopening to attach new patch. 1595691 5 384875 nham 2019-12-04 20:15:28 -0800 Created attachment 384875 Patch 1595692 6 nham 2019-12-04 20:19:11 -0800 How are we testing thread_switch vs sched_yield? There is some difference in using a Mach trap vs. a BSD syscall but I am somewhat surprised it's that large. I have brought up lowering the amount of time that swtch_pri depresses the priority as well, but didn't get any response about that. In any case, we are definitely seeing 10 ms yields on phones (see the radar); whether the calling thread could have woken up earlier is harder to determine from the profile that I was looking at. When the calling thread is the main thread then those 10 ms can count. 1595693 7 nham 2019-12-04 20:26:27 -0800 Actually both swtch_pri and thread_switch seem to be Mach traps. There is a bit more logic in the latter call on the kernel side, but I am somewhat surprised that there would be that much of a difference. But if you have a benchmark in mind that I can try out I would be happy to do that. 1595701 8 fpizlo 2019-12-04 20:42:43 -0800 (In reply to Ben Nham from comment #6) > How are we testing thread_switch vs sched_yield? There is some difference in > using a Mach trap vs. a BSD syscall but I am somewhat surprised it's that > large. > > I have brought up lowering the amount of time that swtch_pri depresses the > priority as well, but didn't get any response about that. > > In any case, we are definitely seeing 10 ms yields on phones (see the > radar); whether the calling thread could have woken up earlier is harder to > determine from the profile that I was looking at. When the calling thread is > the main thread then those 10 ms can count. I don’t think you’re understanding me. Your patch is going to be a performance regression. 1595702 9 384875 fpizlo 2019-12-04 20:43:25 -0800 Comment on attachment 384875 Patch We have tried this change before. It was bad for perf. Unless you’ve got overwhelming perf data, this ain’t a change we are going to land. 1595706 10 fpizlo 2019-12-04 20:46:19 -0800 (In reply to Ben Nham from comment #7) > Actually both swtch_pri and thread_switch seem to be Mach traps. There is a > bit more logic in the latter call on the kernel side, but I am somewhat > surprised that there would be that much of a difference. But if you have a > benchmark in mind that I can try out I would be happy to do that. Any benchmark? We have tried your exact change about 10 times throughout the history of our locks. We have run all the benchmarks. We know it’s a bad idea. 1678932 11 nham 2020-08-08 17:30:27 -0700 We hadn't re-run the benchmarks again since xnu made another change to thread_switch to more closely match the behavior for swtch_pri a couple of years ago. I wasn't able to complete running the benchmarks due to some automation issues, but it looks like Saam was able to do it last week. Duping to Saam's bug, which is the exact same patch. *** This bug has been marked as a duplicate of bug 215248 *** 384875 2019-12-04 20:15:28 -0800 2019-12-04 20:43:25 -0800 Patch bug-204871-20191204201527.patch text/plain 3820 nham U3VidmVyc2lvbiBSZXZpc2lvbjogMjUyOTgyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDZjOTMxZjFkMzIyNjVjNTI2ODU0OWVl MTcyZDJlMWQyYjBlYzc1MGQuLmY1OGFiNGFkY2IyYzAyZDdmMDMzYzYxYmZkMGU1ZTRhOWNkMTY0 ZGQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTktMTItMDQgIEJlbiBOaGFtICA8bmhhbUBhcHBs ZS5jb20+CisKKyAgICAgICAgc2NoZWRfeWllbGQgY2FuIGNhdXNlIGNhbGxpbmcgdGhyZWFkIHRv IGJlIHByZWVtcHRlZCBmb3IgMTAgbXMgb24gRGFyd2luCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDQ4NzEKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVt LzU3NjQ1ODE2PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEN1cnJlbnRseSB3ZSB1c2Ugc2NoZWRfeWllbGQgdG8gYWxsb3cgb3RoZXIgdGhyZWFkcyB0 byBydW4uIFRoaXMgd29ya3MKKyAgICAgICAgYnkgZGVwcmVzc2luZyB0aGUgY2FsbGluZyB0aHJl YWQncyBwcmlvcml0eSB0byAwIGZvciAxIHRpbWVyIHRpY2ssIHdoaWNoCisgICAgICAgIGlzIDEw IG1zIG9uIG1vc3Qgb2Ygb3VyIG1hY2hpbmVzLCB3aGljaCBzZWVtcyB0b28gbG9uZyAoYW5kIGhh cyBzaG93bgorICAgICAgICB1cCBpbiBzb21lIGxhdW5jaCB0aW1lIHByb2ZpbGVzIG9mIE1vYmls ZVNhZmFyaSkuCisKKyAgICAgICAgSW5zdGVhZCwgdXNlIHRocmVhZF9zd2l0Y2gsIHdoaWNoIGRv ZXMgdGhlIHNhbWUgdGhpbmcsIGJ1dCBjYW4gYmUKKyAgICAgICAgY29uZmlndXJlZCB0byBvbmx5 IGRlcHJlc3MgdGhyZWFkIHByaW9yaXR5IGZvciAxIG1zLgorCisgICAgICAgICogd3RmL3Bvc2l4 L1RocmVhZGluZ1BPU0lYLmNwcDoKKyAgICAgICAgKFdURjo6VGhyZWFkOjp5aWVsZCk6CisKIDIw MTktMTEtMjggIEZ1amlpIEhpcm9ub3JpICA8SGlyb25vcmkuRnVqaWlAc29ueS5jb20+CiAKICAg ICAgICAgUmVtb3ZlIEVOQUJMRV9LRVlCT0FSRF9DT0RFX0FUVFJJQlVURSBhbmQgRU5BQkxFX0tF WUJPQVJEX0tFWV9BVFRSSUJVVEUgbWFjcm9zCmRpZmYgLS1naXQgYS9Tb3VyY2UvYm1hbGxvYy9D aGFuZ2VMb2cgYi9Tb3VyY2UvYm1hbGxvYy9DaGFuZ2VMb2cKaW5kZXggZTQ5OGQwN2I3OWRlNGFh YTFjYTEyNjNkMTAyOGZiYTIwZjI4NGRkNi4uZDdmMWY4ZWZlYzhmN2U0MTI5OTQzMjdjNmJkMGMw YzIxMTllMDZjZSAxMDA2NDQKLS0tIGEvU291cmNlL2JtYWxsb2MvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9ibWFsbG9jL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE5LTEyLTA0ICBCZW4g TmhhbSAgPG5oYW1AYXBwbGUuY29tPgorCisgICAgICAgIHNjaGVkX3lpZWxkIGNhbiBjYXVzZSBj YWxsaW5nIHRocmVhZCB0byBiZSBwcmVlbXB0ZWQgZm9yIDEwIG1zIG9uIERhcndpbgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjA0ODcxCisgICAgICAg IDxyZGFyOi8vcHJvYmxlbS81NzY0NTgxNj4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBDdXJyZW50bHkgd2UgdXNlIHNjaGVkX3lpZWxkIHRvIGFsbG93 IG90aGVyIHRocmVhZHMgdG8gcnVuLiBUaGlzIHdvcmtzCisgICAgICAgIGJ5IGRlcHJlc3Npbmcg dGhlIGNhbGxpbmcgdGhyZWFkJ3MgcHJpb3JpdHkgdG8gMCBmb3IgMSB0aW1lciB0aWNrLCB3aGlj aAorICAgICAgICBpcyAxMCBtcyBvbiBtb3N0IG9mIG91ciBtYWNoaW5lcywgd2hpY2ggc2VlbXMg dG9vIGxvbmcgKGFuZCBoYXMgc2hvd24KKyAgICAgICAgdXAgaW4gc29tZSBsYXVuY2ggdGltZSBw cm9maWxlcyBvZiBNb2JpbGVTYWZhcmkpLgorCisgICAgICAgIEluc3RlYWQsIHVzZSB0aHJlYWRf c3dpdGNoLCB3aGljaCBkb2VzIHRoZSBzYW1lIHRoaW5nLCBidXQgY2FuIGJlCisgICAgICAgIGNv bmZpZ3VyZWQgdG8gb25seSBkZXByZXNzIHRocmVhZCBwcmlvcml0eSBmb3IgMSBtcy4KKworICAg ICAgICAqIGJtYWxsb2MvTXV0ZXguY3BwOgorICAgICAgICAoYm1hbGxvYzo6eWllbGRJbXBsKToK KyAgICAgICAgKGJtYWxsb2M6Ok11dGV4Ojpsb2NrU2xvd0Nhc2UpOgorCiAyMDE5LTExLTI1ICBG dWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgogCiAgICAgICAgIFJhbiBz b3J0LVhjb2RlLXByb2plY3QtZmlsZS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL3Bvc2l4 L1RocmVhZGluZ1BPU0lYLmNwcCBiL1NvdXJjZS9XVEYvd3RmL3Bvc2l4L1RocmVhZGluZ1BPU0lY LmNwcAppbmRleCBkZDU1OGQ4NDRlNDNjMmU3ZGY0MTc2OGNkNzdiZDkzMTJlYzVjYjU0Li40Njc5 MjIyYTRmMzNlMzUwM2FmOTc1ZTQ5ZGRiNzU0ODkyZjliYWM0IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V1RGL3d0Zi9wb3NpeC9UaHJlYWRpbmdQT1NJWC5jcHAKKysrIGIvU291cmNlL1dURi93dGYvcG9z aXgvVGhyZWFkaW5nUE9TSVguY3BwCkBAIC00Myw2ICs0MywxMCBAQAogI2luY2x1ZGUgPHd0Zi9U aHJlYWRpbmdQcmltaXRpdmVzLmg+CiAjaW5jbHVkZSA8d3RmL1dvcmRMb2NrLmg+CiAKKyNpZiBP UyhEQVJXSU4pCisjaW5jbHVkZSA8bWFjaC9tYWNoLmg+CisjZW5kaWYKKwogI2lmIE9TKExJTlVY KQogI2luY2x1ZGUgPHN5cy9wcmN0bC5oPgogI2VuZGlmCkBAIC01NTYsNyArNTYwLDExIEBAIHZv aWQgVGhyZWFkQ29uZGl0aW9uOjpicm9hZGNhc3QoKQogCiB2b2lkIFRocmVhZDo6eWllbGQoKQog eworI2lmIE9TKERBUldJTikKKyAgICB0aHJlYWRfc3dpdGNoKE1BQ0hfUE9SVF9OVUxMLCBTV0lU Q0hfT1BUSU9OX0RFUFJFU1MsIDEpOworI2Vsc2UKICAgICBzY2hlZF95aWVsZCgpOworI2VuZGlm CiB9CiAKIH0gLy8gbmFtZXNwYWNlIFdURgpkaWZmIC0tZ2l0IGEvU291cmNlL2JtYWxsb2MvYm1h bGxvYy9NdXRleC5jcHAgYi9Tb3VyY2UvYm1hbGxvYy9ibWFsbG9jL011dGV4LmNwcAppbmRleCA2 NWNlNzc4YTQ3YmMwNjc1NTI3MTcxMTI0YzIxZGNkMjc3MTYwNWNmLi5hODJjOTYyZTVkMTFmY2Yw ZjI1NTQyZTYzNWRkNTQ1NGEzNDZlNDg3IDEwMDY0NAotLS0gYS9Tb3VyY2UvYm1hbGxvYy9ibWFs bG9jL011dGV4LmNwcAorKysgYi9Tb3VyY2UvYm1hbGxvYy9ibWFsbG9jL011dGV4LmNwcApAQCAt MjksOCArMjksMjEgQEAKICNpbmNsdWRlICJTY29wZUV4aXQuaCIKICNpbmNsdWRlIDx0aHJlYWQ+ CiAKKyNpZiBCT1MoREFSV0lOKQorI2luY2x1ZGUgPG1hY2gvbWFjaC5oPgorI2VuZGlmCisKIG5h bWVzcGFjZSBibWFsbG9jIHsKIAorc3RhdGljIHZvaWQgeWllbGRJbXBsKCkKK3sKKyNpZiBCT1Mo REFSV0lOKQorICAgIHRocmVhZF9zd2l0Y2goTUFDSF9QT1JUX05VTEwsIFNXSVRDSF9PUFRJT05f REVQUkVTUywgMSk7CisjZWxzZQorICAgIHNjaGVkX3lpZWxkKCk7CisjZW5kaWYKK30KKwogdm9p ZCBNdXRleDo6bG9ja1Nsb3dDYXNlKCkKIHsKICAgICAvLyBUaGUgbG9uZ2VzdCBjcml0aWNhbCBz ZWN0aW9uIGluIGJtYWxsb2MgaXMgbXVjaCBzaG9ydGVyIHRoYW4gdGhlCkBAIC00OSw3ICs2Miw3 IEBAIHZvaWQgTXV0ZXg6OmxvY2tTbG93Q2FzZSgpCiAKICAgICAvLyBBdm9pZCBzcGlubmluZyBw YXRob2xvZ2ljYWxseS4KICAgICB3aGlsZSAoIXRyeV9sb2NrKCkpCi0gICAgICAgIHNjaGVkX3lp ZWxkKCk7CisgICAgICAgIHlpZWxkSW1wbCgpOwogfQogCiB9IC8vIG5hbWVzcGFjZSBibWFsbG9j Cg==