Bug 124167 – Make DiskImageCache cross-platform

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Gustavo Noronha (kov)]

Make DiskImageCache cross-platform

[Gustavo Noronha (kov)]

Created attachment 216623 [details]
Patch

[Gustavo Noronha (kov)]

So this patch ports the code to POSIX so that we can share it and wires enabling it in WebKit2. I have a patch in the works that makes it possible to map an empty file of a given size to back decoded image data, which should allow for more memory savings than backing the cached resource I hope (still need to test it, will probably get to do that by tomorrow).

[EFL EWS Bot]

Comment on attachment 216623 [details]
Patch

Attachment 216623 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22639483

[EFL EWS Bot]

Comment on attachment 216623 [details]
Patch

Attachment 216623 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/22938276

[Gustavo Noronha (kov)]

Created attachment 216656 [details]
Patch

Fixes EFL build, couple improvements to GTK+ (using the more appropriate XDG user cache directory by default, cleaning up left-overs).

[Joseph Pecoraro]

Comment on attachment 216656 [details]
Patch

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

Overall, the patch looks good to me. The DiskImageCache conversions look good, I'm not qualified to review the WebKit2 or GTK parts, though they look good.

I have one concern, but you're not in a position to be able to test it so I don't expect anything from you in verifying this. Converting from dispatch_queues (in this case dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0)) to threads might have some performance characteristic changes. On iOS, DiskImageCache mappings happen on memory warnings, at a point where memory is already very constrained. Threads are probably heavier weight than the queues, and may have priority differences. However, I don't really know if this would exhibit any problems, this is just a concern.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:13069
> +		FA7094EB7D9A7AE4144F3A05 /* DiskImageCacheClient.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = DiskImageCacheClient.h; sourceTree = "<group>"; };

This will need to be settings = {ATTRIBUTES = (Private, ); like DiskImageCacheClientIOS.h used to be. It is used by WebKit/ios/WebCoreSupport/WebDiskImageCacheClientIOS.h.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20272
>  		CE79D68617F220ED00815C00 /* ios */ = {
>  			isa = PBXGroup;
>  			children = (
> -				A5F9EF6E1266750D00FCCF52 /* DiskImageCacheIOS.mm */,
> -				A5F9EF6F1266750D00FCCF52 /* DiskImageCacheIOS.h */,
> -				A5C566AA127A3AAD00E8A3FF /* DiskImageCacheClientIOS.h */,
>  			);
>  			path = ios;
>  			sourceTree = "<group>";

This group is now empty and could be removed.

> Source/WebCore/platform/DiskImageCache.h:3
> +/*
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> + *

You can include your copyright here as well since you're making some changes.

> Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:142
> +    // Notify the buffer in the case of a successful mapping.
> +    // This should happen on the main thread because this is being run
> +    // asynchronously inside a dispatch queue.

The "dispatch queue" comment is now out of date.

> Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:235
> +    // FIXME: run in main thread when we move the above to its own thread.

Is this FIXME needed anymore? Sounds like you are running the didRemoveFile on the main thread.

> Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:276
> +    // The lifetime of the Entry is handled on the main thread. Before we send
> +    // to a dispatch queue we need to ref so that we are sure the object still
> +    // exists. This call is balanced in the callbacks dispatched from
> +    // Entry::map. or the early return in this dispatch.

More outdated "dispatch queue" related comments.

> Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:295
> +    RefPtr<DiskImageCache::Entry> entry = m_table.get(id);
> +    m_table.remove(id);

Doh. I didn't realize this before, but I think these two lines can just use HashMap<>::take and do one instead of 2 hash lookups:

    RefPtr<DiskImageCache::Entry> entry = m_table.take(id);

Could you drive by fix this?

> Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:340
> +// For GTK+ we use a well-known name inside the user's XDG cache directory, so that we can
> +// better manage its cleanup.

Cleanup is actually what the "didCreateDiskImageCacheDirectory" m_client method is for. However, what you have here is fine, since you will be reusing the same directory over and over. I wonder if it might be easier delete and recreate the directory instead of listing and deleting files one by one.

> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:294
>  #if !PLATFORM(MAC)
> +String createTemporaryDirectory(const String& prefix)

This would likely need !PLATFORM(IOS) around it because FileSystemIOS.mm includes an implementation of createTemporaryDirectory. The IOS implementation uses things like NSTemporaryDirectory() that it likely wants to keep instead of switching to the Posix version.

Maybe the iOS methods will also need to change their signatures. It has NSStrings but it could change to WTF::Strings. You don't need to speculatively fix this since there is no EWS bot for the iOS port.

[Gustavo Noronha (kov)]

Created attachment 216830 [details]
Patch

Thanks for the review, it's very helpful! I did not: a) add the PLATFORM(IOS) check (doesn't the !PLATFORM(MAC) include IOS?) b) change the hash removal to take() - when I did that I started getting asserts in some cases because the entry would have its last ref removed on the last line of DiskImageCache::Entry::removeFileThreadStart, I guess there's some subtle interaction between HashMap::take and RefCounted that I did not grasp, any ideas?

[Gustavo Noronha (kov)]

Here's the assert:

0x00007ffff1e96daf in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:341
341         *(int *)(uintptr_t)0xbbadbeef = 0;
Missing separate debuginfos, use: debuginfo-install google-talkplugin-4.9.1.0-1.x86_64 icedtea-web-1.4.1-0.fc20.x86_64
(gdb) bt
#0  0x00007ffff1e96daf in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:341
#1  0x00007ffff3a96d64 in WebCore::DiskImageCache::Entry::~Entry (this=0xbd4d90, __in_chrg=<optimized out>)
    at ../../Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:79
#2  0x00007ffff3a985a9 in WTF::RefCounted<WebCore::DiskImageCache::Entry>::deref (this=0xbd4d90) at ../../Source/WTF/wtf/RefCounted.h:196
#3  0x00007ffff3a9aaa8 in WTF::RefAndDeref<WebCore::DiskImageCache::Entry*, true>::deref (t=0xbd4d90) at ../../Source/WTF/wtf/Functional.h:412
#4  0x00007ffff3a9aa12 in WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebCore::DiskImageCache::Entry::*)()>, void (WebCore::DiskImageCache::Entry*)>::~BoundFunctionImpl() (this=0x7fff58000970, __in_chrg=<optimized out>) at ../../Source/WTF/wtf/Functional.h:491
#5  0x00007ffff3a9aa4e in WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebCore::DiskImageCache::Entry::*)()>, void (WebCore::DiskImageCache::Entry*)>::~BoundFunctionImpl() (this=0x7fff58000970, __in_chrg=<optimized out>) at ../../Source/WTF/wtf/Functional.h:492
#6  0x00007ffff3a993ea in WTF::ThreadSafeRefCounted<WTF::FunctionImplBase>::deref (this=0x7fff58000978) at ../../Source/WTF/wtf/ThreadSafeRefCounted.h:116
#7  0x00007ffff3a98a93 in WTF::derefIfNotNull<WTF::FunctionImplBase> (ptr=0x7fff58000970) at ../../Source/WTF/wtf/PassRefPtr.h:39
#8  0x00007ffff3a98445 in WTF::RefPtr<WTF::FunctionImplBase>::~RefPtr (this=0x7fff418e4b50, __in_chrg=<optimized out>) at ../../Source/WTF/wtf/RefPtr.h:55
#9  0x00007ffff3a983c0 in WTF::FunctionBase::~FunctionBase (this=0x7fff418e4b50, __in_chrg=<optimized out>) at ../../Source/WTF/wtf/Functional.h:659
#10 0x00007ffff3a983da in WTF::Function<void ()>::~Function() (this=0x7fff418e4b50, __in_chrg=<optimized out>) at ../../Source/WTF/wtf/Functional.h:689
#11 0x00007ffff3a97884 in WebCore::DiskImageCache::Entry::removeFileThreadStart (entryPtr=0xbd4d90)
    at ../../Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:271
#12 0x00007ffff1eb076d in WTF::threadEntryPoint (contextData=0xbdcc40) at ../../Source/WTF/wtf/Threading.cpp:69
#13 0x00007ffff1eb0cf9 in WTF::wtfThreadEntryPoint (param=0xbb8170) at ../../Source/WTF/wtf/ThreadingPthreads.cpp:195
#14 0x0000003ba4207f33 in start_thread (arg=0x7fff418e5700) at pthread_create.c:309
#15 0x0000003ba3af4ead in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

[Build Bot]

Comment on attachment 216830 [details]
Patch

Attachment 216830 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22640090

[Joseph Pecoraro]

(In reply to comment #7)
> Created an attachment (id=216830) [details]
> Patch
> 
> Thanks for the review, it's very helpful! I did not: a) add the PLATFORM(IOS) check (doesn't the !PLATFORM(MAC) include IOS?) b) change the hash removal to take() - when I did that I started getting asserts in some cases because the entry would have its last ref removed on the last line of DiskImageCache::Entry::removeFileThreadStart, I guess there's some subtle interaction between HashMap::take and RefCounted that I did not grasp, any ideas?

Ahh, you might be right. Before the RefPtr you're assigning into can ref the pointer, maybe internally in take the last ref might be going away. I'm not entirely sure, but I think I remember encountering that exact same issue.

[Build Bot]

Comment on attachment 216830 [details]
Patch

Attachment 216830 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22899094

[Gustavo Noronha (kov)]

Created attachment 216914 [details]
Patch

Try to fix my hand-editing of the xcode project file

[Gustavo Noronha (kov)]

Created attachment 221072 [details]
Patch

[WebKit Commit Bot]

Attachment 221072 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/loader/cache/MemoryCache.cpp', u'Source/WebCore/loader/ios/DiskImageCacheClientIOS.h', u'Source/WebCore/loader/ios/DiskImageCacheIOS.h', u'Source/WebCore/loader/ios/DiskImageCacheIOS.mm', u'Source/WebCore/platform/DiskImageCache.h', u'Source/WebCore/platform/DiskImageCacheClient.h', u'Source/WebCore/platform/FileSystem.h', u'Source/WebCore/platform/SharedBuffer.cpp', u'Source/WebCore/platform/gtk/FileSystemGtk.cpp', u'Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp', u'Source/WebCore/platform/posix/FileSystemPOSIX.cpp', u'Source/WebCore/platform/soup/SharedBufferSoup.cpp', u'Source/WebKit/ios/ChangeLog', u'Source/WebKit/ios/WebCoreSupport/WebDiskImageCacheClientIOS.h', u'Source/autotools/SetupWebKitFeatures.m4', u'Tools/ChangeLog', u'Tools/Scripts/webkitperl/FeatureList.pm', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:106:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:108:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/posix/DiskImageCachePOSIX.cpp:115:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

I'm pretty sure the style issues are false-positives, I think the style checker is misparsing that part of the code.

[Gustavo Noronha (kov)]

Ping, how should I move forward with this? I guess one thing would be to revisit the PLATFORM(IOS) bit after the PLATFORM(MAC) changes came through?

[Joseph Pecoraro]

(In reply to comment #16)
> Ping, how should I move forward with this? I guess one thing would be to revisit the PLATFORM(IOS) bit after the PLATFORM(MAC) changes came through?

Good question. Is EFL interesting in enabling this soon? If so, we should get a review and probably coordinate with someone on iOS to ensure after landing that it still behaves as expected on iOS. That coordination is the tricky part =).

[Joseph Pecoraro]

EFL or GTK*

[Andre Moreira Magalhaes]

Closing in favor of bug #148028.