Bug 47345 - [Qt] Implement SharedMemory for WebKit2
Summary: [Qt] Implement SharedMemory for WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on: 48238
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-07 06:52 PDT by Zoltan Horvath
Modified: 2010-10-26 09:22 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (21.64 KB, patch)
2010-10-07 06:54 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch (21.64 KB, patch)
2010-10-07 07:19 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated proposed patch (21.68 KB, patch)
2010-10-07 09:53 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
proposed patch (21.68 KB, patch)
2010-10-07 10:13 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
updated patch (22.11 KB, patch)
2010-10-15 05:23 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
svn patch (16.74 KB, patch)
2010-10-15 08:37 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
svn patch (22.17 KB, patch)
2010-10-15 08:51 PDT, Zoltan Horvath
koivisto: review-
Details | Formatted Diff | Diff
proposed patch (27.82 KB, patch)
2010-10-25 02:30 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2010-10-07 06:52:30 PDT
Implement unimplemented functions in SharedMemoryQt.cpp.
Move MappedMemory.h and MappedMemoryPool.cpp from Shared/qt to Platform/qt, change WebKit2.pro's affected lines.
Comment 1 Zoltan Horvath 2010-10-07 06:54:48 PDT
Created attachment 70077 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-10-07 06:55:30 PDT
Attachment 70077 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/UpdateChunk.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zoltan Horvath 2010-10-07 07:19:32 PDT
Created attachment 70086 [details]
updated proposed patch

I removed unnecessary headers.
I'll open a new bug to remove global functions and make UpdateChunk to use SharedMemory.
Comment 4 Balazs Kelemen 2010-10-07 08:08:22 PDT
Comment on attachment 70086 [details]
updated proposed patch

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

> WebKit2/Platform/qt/SharedMemoryQt.cpp:170
> +
> +    for (unsigned n = 0; n < pool->size(); ++n) {
> +        MappedMemory mm = pool->at(n);
> +        *current = mm;

There is no need to allocating a MappedMemory for storing the result. You should do it like 'current = &pool->at(n)'

> WebKit2/Platform/qt/SharedMemoryQt.cpp:173
> +
> +    for (unsigned n = 0; n < pool->size(); ++n) {
> +        MappedMemory mm = pool->at(n);
> +        *current = mm;
> +        if (current->data == p) 
> +            return current;
> +    }

You have almost the same for in mapFile and mapMemory. All of these should be merged to a static that returns
with a pointer and you can do what you want to do with the pointer at the caller site.

> WebKit2/Platform/qt/SharedMemoryQt.cpp:192
> +
> +    QString file = searchForMappedMemory(reinterpret_cast<uchar*>(m_data))->file->fileName();

Here you should check for 0 pointer or asserting that the result is not zero. I think it should be never be zero here.
Comment 5 Zoltan Horvath 2010-10-07 09:53:50 PDT
Created attachment 70100 [details]
updated proposed patch

(In reply to comment #4)
> There is no need to allocating a MappedMemory for storing the result. You should do it like 'current = &pool->at(n)'

It's true. Done.

> You have almost the same for in mapFile and mapMemory. All of these should be merged to a static that returns
> with a pointer and you can do what you want to do with the pointer at the caller site.

Unfortunately, we can't merge them, these needs to do different things.
 
> Here you should check for 0 pointer or asserting that the result is not zero. I think it should be never be zero here.

I added a null check.
Comment 6 WebKit Review Bot 2010-10-07 09:55:16 PDT
Attachment 70100 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/UpdateChunk.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Platform/qt/SharedMemoryQt.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Platform/qt/SharedMemoryQt.cpp:182:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Zoltan Horvath 2010-10-07 10:13:43 PDT
Created attachment 70107 [details]
proposed patch

Style issues solved.
Comment 8 Balazs Kelemen 2010-10-07 10:22:09 PDT
Comment on attachment 70107 [details]
proposed patch

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

> WebKit2/Platform/qt/SharedMemoryQt.cpp:198
> +
> +    QString file;   
> +
> +    if (mm)
> +        file = mm->file->fileName();
> +
> +    if (file.isEmpty())
> +        return false;

Why not just returning early if mm is zero? You can fix that before landing I think.
Comment 9 Kenneth Rohde Christiansen 2010-10-08 05:49:19 PDT
I really think you should discuss this patch with Antti.
Comment 10 Zoltan Horvath 2010-10-08 06:06:39 PDT
Okay, then I'm waiting for Antti. 
Btw, I think the correct way of WebKit developing is to sending patches into bugzilla. Fortunately, my actual changes wouldn't cause much rebasing trouble for Antti.
Comment 11 Zoltan Horvath 2010-10-15 05:23:44 PDT
Created attachment 70856 [details]
updated patch

Updated to latest.
Ping for r!
Comment 12 Zoltan Horvath 2010-10-15 08:37:35 PDT
Created attachment 70869 [details]
svn patch
Comment 13 Early Warning System Bot 2010-10-15 08:46:53 PDT
Attachment 70869 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4423050
Comment 14 Zoltan Horvath 2010-10-15 08:51:07 PDT
Created attachment 70871 [details]
svn patch

This contains the svn mv infos.
Comment 15 Kenneth Rohde Christiansen 2010-10-15 12:33:45 PDT
Antti, could you comment on this?
Comment 16 Antti Koivisto 2010-10-19 06:16:31 PDT
Comment on attachment 70871 [details]
svn patch

We discussed this on irc. The direction looks good but I would prefer a patch with more completed refactoring.
 
- MappedMemory.h has two classes and it is not clear which interfaces are public
- MappedMemory.h vs. impl in MappedMemoryPool.cpp
- Would it reduce confusion to move MappedMemory code to SharedMemoryQt.cpp and make UpdateChunk use SharedMemory?
- as is, mapFile/mapMemory should be declared in header, not in .cpp
- mapFile/mapMemory should probably be in class scope
- MappedMemoryPool::cleanUp() (and possibly the whole class) might be unnecessary since we unlink the shared memory files right after mapping.
- Nokia copyrights should be maintained when copying code to new files
Comment 17 Zoltan Horvath 2010-10-25 02:30:42 PDT
Created attachment 71722 [details]
proposed patch

(In reply to comment #16)
> (From update of attachment 70871 [details])
> We discussed this on irc. The direction looks good but I would prefer a patch with more completed refactoring.
> 
> - MappedMemory.h has two classes and it is not clear which interfaces are public

I renamed MappedMemory.h to MappedMemoryPool.h.

> - MappedMemory.h vs. impl in MappedMemoryPool.cpp

I put MappedMemory implementation into MappedMemoryPool.

> - Would it reduce confusion to move MappedMemory code to SharedMemoryQt.cpp and make UpdateChunk use SharedMemory?

I didn't do this. In this mod, UpdateChunk uses MappedMemoryPool.

> - as is, mapFile/mapMemory should be declared in header, not in .cpp
> - mapFile/mapMemory should probably be in class scope
> - MappedMemoryPool::cleanUp() (and possibly the whole class) might be unnecessary since we unlink the shared memory files right after mapping.

I put mapFile/mapMemory into MappedMemoryPool and removed unnecessary methods.

> - Nokia copyrights should be maintained when copying code to new files

Done.
Comment 18 WebKit Review Bot 2010-10-25 02:34:56 PDT
Attachment 71722 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Platform/qt/MappedMemoryPool.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Kenneth Rohde Christiansen 2010-10-25 03:23:54 PDT
Comment on attachment 71722 [details]
proposed patch

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

> WebKit2/Platform/qt/MappedMemoryPool.h:36
> +struct MappedMemory {

Here

> WebKit2/Platform/qt/MappedMemoryPool.h:40
> +    struct MappedMemory {

Why is this defined twice?
Comment 20 Kenneth Rohde Christiansen 2010-10-25 03:24:38 PDT
(In reply to comment #19)
> (From update of attachment 71722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71722&action=review
> 
> > WebKit2/Platform/qt/MappedMemoryPool.h:36
> > +struct MappedMemory {
> 
> Here
> 
> > WebKit2/Platform/qt/MappedMemoryPool.h:40
> > +    struct MappedMemory {
> 
> Why is this defined twice?

Hmm, it is like the patch is showing the same file twise!
Comment 21 Zoltan Horvath 2010-10-25 05:28:31 PDT
(In reply to comment #20)

> Hmm, it is like the patch is showing the same file twise!

It caused by the svn move and after the changes, didn't it?
Comment 22 Kenneth Rohde Christiansen 2010-10-25 05:30:35 PDT
Comment on attachment 71722 [details]
proposed patch

Maybe :-) just be careful when landing!
Comment 23 Zoltan Horvath 2010-10-25 06:57:02 PDT
Thanks for the review!
Patch landed successfully.
Committed revision 70450.
http://trac.webkit.org/changeset/70450
Comment 24 Zoltan Horvath 2010-10-26 08:03:08 PDT
The guys rolled out the patch in r70457.
It contained 1 wrong line in SharedMemoryQt.cpp. (line 128) 
I removed the wrong line and reland the patch in r70520.