Bug 225368 - Streamline codec parsing, replacing uses of HashMap with SortedArrayMap
Summary: Streamline codec parsing, replacing uses of HashMap with SortedArrayMap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P3 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-04 15:18 PDT by Darin Adler
Modified: 2021-05-06 16:57 PDT (History)
12 users (show)

See Also:


Attachments
Patch (62.89 KB, patch)
2021-05-04 15:55 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (67.90 KB, patch)
2021-05-04 18:58 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (67.90 KB, patch)
2021-05-04 19:46 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2021-05-04 15:18:45 PDT
Streamline codec parsing, replacing uses of HashMap with SortedArrayMap
Comment 1 Darin Adler 2021-05-04 15:55:40 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2021-05-04 18:58:41 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2021-05-04 19:46:00 PDT
Created attachment 427727 [details]
Patch
Comment 4 Darin Adler 2021-05-04 22:49:55 PDT
Passing all the EWS tests, so now looking for a reviewer.
Comment 5 Sam Weinig 2021-05-06 08:36:40 PDT
Comment on attachment 427727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427727&action=review

> Source/WebCore/platform/graphics/HEVCUtilities.cpp:128
> +    return makeOptionalFromPointer(typesMap.tryGet(string));

Should we just make tryGet() return an Optional? Or, perhaps have a version of tryGet that does? This seems like it could be a pretty common idiom? (though, as always, this kind of change can and should be left until there is actual evidence that we need to have duplicate copies of makeOptionalFromPointer()).

> Source/WebCore/platform/graphics/cocoa/HEVCUtilitiesCocoa.mm:150
> +    Vector<uint16_t> vector;
> +    vector.reserveInitialCapacity(array.count);
> +    for (id value in array) {

I think you could use compactMap() here. If you don't want to (or it doesn't work), a shinkToFit() after the loop is probably a good idea here.
Comment 6 Darin Adler 2021-05-06 08:48:40 PDT
Comment on attachment 427727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427727&action=review

>> Source/WebCore/platform/graphics/HEVCUtilities.cpp:128
>> +    return makeOptionalFromPointer(typesMap.tryGet(string));
> 
> Should we just make tryGet() return an Optional? Or, perhaps have a version of tryGet that does? This seems like it could be a pretty common idiom? (though, as always, this kind of change can and should be left until there is actual evidence that we need to have duplicate copies of makeOptionalFromPointer()).

I do keep running into the intersection between Optional and pointers; in a lot of cases the two are used almost interchangeably but they are not all that easy to convert between.

I can’t decide which is better for tryGet; the pointer lets us peek into the array, but the Optional has more clean object lifetime semantics for callers that don’t know about an underlying array as an implementation detail.

>> Source/WebCore/platform/graphics/cocoa/HEVCUtilitiesCocoa.mm:150
>> +    for (id value in array) {
> 
> I think you could use compactMap() here. If you don't want to (or it doesn't work), a shinkToFit() after the loop is probably a good idea here.

I don’t think either of those is right for this. This does not skip over invalid values, instead it fails and drops the vector if it encounters an invalid value. So the vector is always perfectly sized because of reserving the initial capacity and a shrink to fit is not needed.
Comment 7 EWS 2021-05-06 09:08:25 PDT
Committed r277093 (237395@main): <https://commits.webkit.org/237395@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427727 [details].
Comment 8 Radar WebKit Bug Importer 2021-05-06 09:09:20 PDT
<rdar://problem/77609399>
Comment 9 Sam Weinig 2021-05-06 16:57:21 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 427727 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427727&action=review
> 
> 
> >> Source/WebCore/platform/graphics/cocoa/HEVCUtilitiesCocoa.mm:150
> >> +    for (id value in array) {
> > 
> > I think you could use compactMap() here. If you don't want to (or it doesn't work), a shinkToFit() after the loop is probably a good idea here.
> 
> I don’t think either of those is right for this. This does not skip over
> invalid values, instead it fails and drops the vector if it encounters an
> invalid value. So the vector is always perfectly sized because of reserving
> the initial capacity and a shrink to fit is not needed.


Oops. Indeed.