Bug 153910 - Clean up Blob code
Summary: Clean up Blob code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-04 21:28 PST by Alex Christensen
Modified: 2016-02-05 08:57 PST (History)
0 users

See Also:


Attachments
Patch (16.88 KB, patch)
2016-02-04 21:37 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (22.80 KB, patch)
2016-02-04 22:42 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (20.34 KB, patch)
2016-02-04 22:51 PST, Alex Christensen
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-02-04 21:28:28 PST
Clean up Blob code
Comment 1 Alex Christensen 2016-02-04 21:37:58 PST
Created attachment 270725 [details]
Patch
Comment 2 Alex Christensen 2016-02-04 22:42:51 PST
Created attachment 270736 [details]
Patch
Comment 3 Alex Christensen 2016-02-04 22:51:30 PST
Created attachment 270737 [details]
Patch
Comment 4 Alexey Proskuryakov 2016-02-04 23:01:09 PST
Comment on attachment 270737 [details]
Patch

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

> Source/WebCore/fileapi/Blob.h:79
> +#ifndef NDEBUG

If this is for assertions only, then it should be using ASSERT_DISABLED macro (name from memory).

> Source/WebCore/platform/network/BlobRegistry.h:69
> +    virtual ~BlobRegistry() { };

This is not an improvement - constructors and destructors of polymorphic classes should not be inline, because of code size bloat.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:78
> +        return nullptr;

Does return value optimization work in functions with multiple exit points?

> Source/WebCore/platform/network/BlobRegistryImpl.h:70
> +    HashMap<URL, RefPtr<BlobData>> m_blobs;

Why is this better? URL is a large object. Do we waste time reparsing them later?
Comment 5 Alex Christensen 2016-02-05 08:46:15 PST
Comment on attachment 270737 [details]
Patch

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

>> Source/WebCore/platform/network/BlobRegistryImpl.cpp:78
>> +        return nullptr;
> 
> Does return value optimization work in functions with multiple exit points?

Yes.

> Source/WebCore/platform/network/BlobRegistryImpl.cpp:149
> -            if (!m_blobs.contains(part.url().string()))
> +            if (!m_blobs.contains(part.url()))
>                  return;
> -            for (const BlobDataItem& item : m_blobs.get(part.url().string())->items())
> +            for (const BlobDataItem& item : m_blobs.get(part.url())->items())

I am going to make this only lookup the url once in the HashMap, though.

>> Source/WebCore/platform/network/BlobRegistryImpl.h:70
>> +    HashMap<URL, RefPtr<BlobData>> m_blobs;
> 
> Why is this better? URL is a large object. Do we waste time reparsing them later?

I thought it was more clear, and we don't reparse them, but you're right: this is a memory regression.
Comment 6 Alex Christensen 2016-02-05 08:57:26 PST
http://trac.webkit.org/changeset/196174