234940 2022-01-06 17:16:49 -0800 DataURLResourceMediaLoader decodes the URL repeatedly during a video playback 2022-01-18 12:42:07 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=235325 InRadar P2 Normal --- 1 peng.liu6 peng.liu6 darin eric.carlson jean-yves.avenard jer.noble webkit-bug-importer oldest_to_newest 1828783 0 peng.liu6 2022-01-06 17:16:49 -0800 DataURLResourceMediaLoader decodes the URL repeatedly during a video playback 1828787 1 448550 peng.liu6 2022-01-06 17:37:11 -0800 Created attachment 448550 [fast-cq] Patch 1828788 2 peng.liu6 2022-01-06 17:38:06 -0800 <rdar://83925935> 1828798 3 448550 jean-yves.avenard 2022-01-06 18:27:01 -0800 Comment on attachment 448550 [fast-cq] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=448550&action=review > Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:365 > + if (!m_dataURLMediaLoader && [contentInfo respondsToSelector:@selector(setEntireLengthAvailableOnDemand:)]) couldn't we cache the URL decoding instead and re-use that ? 1828925 4 peng.liu6 2022-01-07 09:00:40 -0800 (In reply to Jean-Yves Avenard [:jya] from comment #3) > Comment on attachment 448550 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448550&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:365 > > + if (!m_dataURLMediaLoader && [contentInfo respondsToSelector:@selector(setEntireLengthAvailableOnDemand:)]) > > couldn't we cache the URL decoding instead and re-use that ? Actually, this is what this patch does. AVFoundation caches the decoded result internally. Some refactoring is needed if we want to cache the result in WebKit. 1829869 5 448550 darin 2022-01-11 11:19:59 -0800 Comment on attachment 448550 [fast-cq] Patch Can we find a way to test this? 1829937 6 peng.liu6 2022-01-11 15:22:37 -0800 (In reply to Darin Adler from comment #5) > Comment on attachment 448550 [details] > Patch > > Can we find a way to test this? This patch fixes a performance issue. I manually tested it. But I am not sure we can add a reliable regression test for it. 1829939 7 ews-feeder 2022-01-11 15:26:24 -0800 Committed r287899 (245936@main): <https://commits.webkit.org/245936@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 448550. 1829959 8 darin 2022-01-11 16:34:13 -0800 We have other tests for performance issues. I’m surprised to hear we can’t make a test for this one. 1829966 9 peng.liu6 2022-01-11 17:04:54 -0800 (In reply to Darin Adler from comment #8) > We have other tests for performance issues. I’m surprised to hear we can’t > make a test for this one. Do you mean regression tests which run automatically on bots? 1829971 10 darin 2022-01-11 17:41:00 -0800 Yes. There are a few in LayoutTests/perf for example. 1830151 11 peng.liu6 2022-01-12 08:59:30 -0800 (In reply to Darin Adler from comment #10) > Yes. There are a few in LayoutTests/perf for example. Thanks! These tests look useful! However, the test for this patch is more difficult. Because the performance issue is related to AVFoundation's internal behavior, and there is no obvious way to get the performance difference (before and after applying this patch) from JS code. 1830187 12 darin 2022-01-12 09:59:01 -0800 I’m willing to accept no for an answer. But I really don’t understand why we can’t create a test; every kind of test we have originally did not exist, and was invented because we needed to protect the performance or reliability of WebKit in the long run. Presumably we can make a huge stress test case where the performance impact is very obvious, and make a way to run it. Given the fix here is subtle, and depends on both WebKit and AVFoundation, I think it’s really risky to just make the fix without creating a regression test. 1831771 13 peng.liu6 2022-01-18 12:42:07 -0800 (In reply to Darin Adler from comment #12) > I’m willing to accept no for an answer. > > But I really don’t understand why we can’t create a test; every kind of test > we have originally did not exist, and was invented because we needed to > protect the performance or reliability of WebKit in the long run. Presumably > we can make a huge stress test case where the performance impact is very > obvious, and make a way to run it. > > Given the fix here is subtle, and depends on both WebKit and AVFoundation, I > think it’s really risky to just make the fix without creating a regression > test. I am working on a regression test now. Eric and Jer provided some good suggestions. https://bugs.webkit.org/show_bug.cgi?id=235325 448550 2022-01-06 17:37:11 -0800 2022-01-11 15:26:26 -0800 [fast-cq] Patch bug-234940-20220106173710.patch text/plain 2991 peng.liu6 U3VidmVyc2lvbiBSZXZpc2lvbjogMjg3NjcyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMmVmZDZkNDY3NWVlYTcx ODdlN2YzMzA4YjFkN2JiNzY3OWNlZjZhOS4uNWQzZTY1ZjIxZDgxZjNkMzNmOTAzZjI5YWU5ODVk YjZmYTI1YTAwOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDIyLTAxLTA2ICBQZW5n IExpdSAgPHBlbmcubGl1NkBhcHBsZS5jb20+CisKKyAgICAgICAgRGF0YVVSTFJlc291cmNlTWVk aWFMb2FkZXIgZGVjb2RlcyB0aGUgVVJMIHJlcGVhdGVkbHkgZHVyaW5nIGEgdmlkZW8gcGxheWJh Y2sKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIzNDk0 MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEluIHIy NjQ4NjQsIHdlIGFkb3B0ZWQgYSBuZXcgU1BJIHRvIHRlbGwgQVZGb3VuZGF0aW9uIHRoYXQgdGhl IGVudGlyZSBmaWxlIGlzIGF2YWlsYWJsZSBmb3IgY3VzdG9tIFVSTHMuCisgICAgICAgIEFzIGEg cmVzdWx0LCBkdXJpbmcgYSB2aWRlbyBwbGF5YmFjaywgQVZGb3VuZGF0aW9uIHdpbGwgcmVxdWVz dCBzbWFsbCBkYXRhIHJhbmdlcyBpbnN0ZWFkIG9mICJjYWNoaW5nIgorICAgICAgICB0aGUgd2hv bGUgZmlsZSBhZ2Fpbi4KKworICAgICAgICBIb3dldmVyLCB0aGF0IGxlYWRzIHRvIGVmZmljaWVu Y3kgaXNzdWUgZm9yIERhdGFVUkxSZXNvdXJjZU1lZGlhTG9hZGVyLiBCZWNhdXNlIGl0IG5lZWRz IHRvIGRlY29kZSB0aGUgd2hvbGUKKyAgICAgICAgVVJMIHdoZW4gQVZGb3VuZGF0aW9uIHJlcXVl c3RzIGEgZGF0YSByYW5nZSwgd2hpY2ggaXMgaW5lZmZpY2llbnQgd2hlbiB0aGUgVVJMIGlzIHZl cnkgbG9uZy4KKworICAgICAgICBUaGlzIHBhdGNoIHJldmVydHMgdGhlIGNoYW5nZSBpbiByMjY0 ODY0IGZvciB0aGUgRGF0YVVSTFJlc291cmNlTWVkaWFMb2FkZXIgY2FzZSB0byBmaXggdGhlIHBl cmZvcm1hbmNlIGlzc3VlLgorICAgICAgICBJZiBEYXRhVVJMRGVjb2Rlcjo6ZGVjb2RlKCkgc3Vw cG9ydHMgZGVjb2RpbmcgYSBkYXRhIHJhbmdlIGluIHRoZSBmdXR1cmUsIHdlIGNhbiBjaGFuZ2Ug aXQgYmFjayBmb3IgYmV0dGVyCisgICAgICAgIG1lbW9yeSBlZmZpY2llbmN5LgorCisgICAgICAg ICogcGxhdGZvcm0vZ3JhcGhpY3MvYXZmb3VuZGF0aW9uL29iamMvV2ViQ29yZUFWRlJlc291cmNl TG9hZGVyLm1tOgorICAgICAgICAoV2ViQ29yZTo6V2ViQ29yZUFWRlJlc291cmNlTG9hZGVyOjpy ZXNwb25zZVJlY2VpdmVkKToKKwogMjAyMi0wMS0wNSAgV2Vuc29uIEhzaWVoICA8d2Vuc29uX2hz aWVoQGFwcGxlLmNvbT4KIAogICAgICAgICBNb2RhbCBjb250YWluZXIgb2JzZXJ2ZXIgc2hvdWxk IGRldGVjdCBhbmQgc3VwcHJlc3MgZWxlbWVudHMgdGhhdCBwcmV2ZW50IHVzZXIgaW50ZXJhY3Rp b24KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2F2Zm91bmRh dGlvbi9vYmpjL1dlYkNvcmVBVkZSZXNvdXJjZUxvYWRlci5tbSBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL2F2Zm91bmRhdGlvbi9vYmpjL1dlYkNvcmVBVkZSZXNvdXJjZUxvYWRl ci5tbQppbmRleCBmOTIxZjNkYTBlNjliMzM5OTc3YzllM2U0OTY2N2EyOGY1ZTk1Y2JkLi45MGRh NjhlZDM1NzEyZGUzZDJlMjhkNmRkZTA2OTczNTUzODJiOGM4IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9hdmZvdW5kYXRpb24vb2JqYy9XZWJDb3JlQVZGUmVz b3VyY2VMb2FkZXIubW0KKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvYXZm b3VuZGF0aW9uL29iamMvV2ViQ29yZUFWRlJlc291cmNlTG9hZGVyLm1tCkBAIC0zNTcsOCArMzU3 LDEyIEBAIHZvaWQgV2ViQ29yZUFWRlJlc291cmNlTG9hZGVyOjpyZXNwb25zZVJlY2VpdmVkKGNv bnN0IFJlc291cmNlUmVzcG9uc2UmIHJlc3BvbnNlCiAKICAgICAgICAgW2NvbnRlbnRJbmZvIHNl dENvbnRlbnRMZW5ndGg6Y29udGVudFJhbmdlLmlzVmFsaWQoKSA/IGNvbnRlbnRSYW5nZS5pbnN0 YW5jZUxlbmd0aCgpIDogcmVzcG9uc2UuZXhwZWN0ZWRDb250ZW50TGVuZ3RoKCldOwogICAgICAg ICBbY29udGVudEluZm8gc2V0Qnl0ZVJhbmdlQWNjZXNzU3VwcG9ydGVkOllFU107Ci0gICAgICAg IAotICAgICAgICBpZiAoW2NvbnRlbnRJbmZvIHJlc3BvbmRzVG9TZWxlY3RvcjpAc2VsZWN0b3Io c2V0RW50aXJlTGVuZ3RoQXZhaWxhYmxlT25EZW1hbmQ6KV0pCisKKyAgICAgICAgLy8gRG8gbm90 IHNldCAiRW50aXJlTGVuZ3RoQXZhaWxhYmxlT25EZW1hbmQiIHRvIFlFUyB3aGVuIHRoZSBsb2Fk ZXIgaXMgRGF0YVVSTFJlc291cmNlTWVkaWFMb2FkZXIuCisgICAgICAgIC8vIFdoZW4gdGhlIHBy b3BlcnR5IGlzIFlFUywgQVZBc3NldFJlc291cmNlTG9hZGVyIHdpbGwgcmVxdWVzdCBzbWFsbCBk YXRhIHJhbmdlcyBvdmVyIGFuZCBvdmVyIGFnYWluCisgICAgICAgIC8vIGR1cmluZyB0aGUgcGxh eWJhY2suIEZvciBEYXRhVVJMUmVzb3VyY2VNZWRpYUxvYWRlciwgdGhhdCBtZWFucyBpdCBuZWVk cyB0byBkZWNvZGUgdGhlIFVSTCByZXBlYXRlZGx5LAorICAgICAgICAvLyB3aGljaCBpcyB2ZXJ5 IGluZWZmaWNpZW50IGZvciBsb25nIFVSTHMuCisgICAgICAgIGlmICghbV9kYXRhVVJMTWVkaWFM b2FkZXIgJiYgW2NvbnRlbnRJbmZvIHJlc3BvbmRzVG9TZWxlY3RvcjpAc2VsZWN0b3Ioc2V0RW50 aXJlTGVuZ3RoQXZhaWxhYmxlT25EZW1hbmQ6KV0pCiAgICAgICAgICAgICBbY29udGVudEluZm8g c2V0RW50aXJlTGVuZ3RoQXZhaWxhYmxlT25EZW1hbmQ6WUVTXTsKIAogICAgICAgICBpZiAoIVtt X2F2UmVxdWVzdCBkYXRhUmVxdWVzdF0pIHsK