Bug 49791

Created attachment 74365 [details] Implements shared memory for GTK port

Created attachment 78229 [details] Shared Memory Implementation on latest revision

Comment on attachment 78229 [details] Shared Memory Implementation on latest revision View in context: https://bugs.webkit.org/attachment.cgi?id=78229&action=review Very nice start! > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:81 > + : m_fileName() No need to use an empty initializer here. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:121 > + char fileName[] = "/tmp/WK2SharedMemoryXXXXXX"; > + if (mkstemp(fileName) == -1) // create a unique name > + return 0; Please use GLib functions to do this. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:126 > + RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory)); > + sharedMemory->m_size = size; > + sharedMemory->m_data = createShareableMemory(fileName, size); > + sharedMemory->m_fileName = String(fileName); I would much prefer these to be arguments to the constructor, if possible. It's an error here to use cast a char* to a String. You should use filenameToString (from FileSystemGtk) or use a CString a m_fileName, if possible. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:149 > + int mode = memoryProtection(protection); You only use this once, so no need to cache it in "mode" > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:150 > + char* fileName = const_cast<char*>(handle.m_fileName.utf8().data()); You cannot assume UTF-8 here. See above. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:155 > + RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory)); > + sharedMemory->m_size = size; > + sharedMemory->m_data = obtainSharedMemory(fileName, size, mode); Again, I think it's better to pass these as arguments to the constructor. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:158 > + handle.m_fileName = String(); Why is this necessary? It should probably have a comment explaining it.

Created attachment 78527 [details] Addressed the review comments

(In reply to comment #3) > (From update of attachment 78229 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78229&action=review > > Very nice start! > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:81 > > + : m_fileName() > > No need to use an empty initializer here. Done. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:121 > > + char fileName[] = "/tmp/WK2SharedMemoryXXXXXX"; > > + if (mkstemp(fileName) == -1) // create a unique name > > + return 0; > > Please use GLib functions to do this. Done. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:126 > > + RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory)); > > + sharedMemory->m_size = size; > > + sharedMemory->m_data = createShareableMemory(fileName, size); > > + sharedMemory->m_fileName = String(fileName); > > I would much prefer these to be arguments to the constructor, if possible. It's an error here to use cast a char* to a String. You should use filenameToString (from FileSystemGtk) or use a CString a m_fileName, if possible. The port independent SharedMemory class doesn't define any constructor. Hence, I took this approach. Other ports implement the same way. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:149 > > + int mode = memoryProtection(protection); > > You only use this once, so no need to cache it in "mode" Done. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:150 > > + char* fileName = const_cast<char*>(handle.m_fileName.utf8().data()); > > You cannot assume UTF-8 here. See above. Done. I used fileSystemRepresentation over here. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:155 > > + RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory)); > > + sharedMemory->m_size = size; > > + sharedMemory->m_data = obtainSharedMemory(fileName, size, mode); > > Again, I think it's better to pass these as arguments to the constructor. Same as above. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:158 > > + handle.m_fileName = String(); > > Why is this necessary? It should probably have a comment explaining it. Removed this.

Comment on attachment 78527 [details] Addressed the review comments View in context: https://bugs.webkit.org/attachment.cgi?id=78527&action=review Looks good, but consider using GLib functions. If it's not possible, I'll re-review. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:55 > + if ((lseek(fd, size - 1, SEEK_SET) == -1) || (write(fd, "", 1) != 1)) { > + close(fd); > + return 0; > + } Is this just verifying that the file is writable? > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:61 > + void* mappedMemory = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > + if (mappedMemory == MAP_FAILED) { > + close(fd); > + return 0; > + } Is it possible to use GMappedFile here? I think it makes sense to use GLib function wherever possible. This will make this function platform independent and also prevent the need for the above code. > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:77 > + void* mappedMemory = mmap(0, size, mode, MAP_SHARED, fd, 0); > + if (mappedMemory == MAP_FAILED) { > + close(fd); > + return 0; > + } Ditto.

Created attachment 78694 [details] Used GLib API for mmap

(In reply to comment #6) > (From update of attachment 78527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78527&action=review > > Looks good, but consider using GLib functions. If it's not possible, I'll re-review. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:55 > > + if ((lseek(fd, size - 1, SEEK_SET) == -1) || (write(fd, "", 1) != 1)) { > > + close(fd); > > + return 0; > > + } > > Is this just verifying that the file is writable? It sets the size of the file by moving size bytes and writing a character at the end. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:61 > > + void* mappedMemory = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); > > + if (mappedMemory == MAP_FAILED) { > > + close(fd); > > + return 0; > > + } > > Is it possible to use GMappedFile here? I think it makes sense to use GLib function wherever possible. This will make this function platform independent and also prevent the need for the above code. Done. Used GMappedFile with the new patch. > > > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:77 > > + void* mappedMemory = mmap(0, size, mode, MAP_SHARED, fd, 0); > > + if (mappedMemory == MAP_FAILED) { > > + close(fd); > > + return 0; > > + } > > Ditto. Done.

Comment on attachment 78694 [details] Used GLib API for mmap View in context: https://bugs.webkit.org/attachment.cgi?id=78694&action=review Unofficial review > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:48 > + GIOChannel* ioChannel = g_io_channel_new_file(fileName, "w+", NULL); > + if (!ioChannel) > + return 0; Instead of using GIOChannel, it might be easier to use gio api and use g_seekable_truncate(). It might be something like: GRefPtr<GFile> tmpFile = g_file_new_for_path(fileName); GRefPtr<GFileIOStream> fileStream = g_file_open_readwrite(tmpFile.get(), 0, 0); // You might want to use a GError to handle errors here g_seekable_truncate (G_SEEKABLE (fileStream.get()), size, 0, 0); // You might want to use a GError to handle errors here g_io_stream_close(G_IO_STREAM(fileStream.get()), 0, 0); // You might want to use a GError to handle errors here > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:65 > + GMappedFile* mappedFile = g_mapped_file_new(fileName, TRUE, NULL); > + if (!mappedFile) > + return 0; When g_mapped_file_new() fails it returns NULL, so you can simply return g_mapped_file_new(fileName, TRUE, 0); > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:74 > + GMappedFile* mappedFile = g_mapped_file_new(fileName, writable, NULL); > + if (!mappedFile) > + return 0; The same here, I think we don't even need this method, we can just use g_mapped_file_new() directly > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:117 This is not portable, use g_get_tmp_dir() > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:118 > + if (g_mkstemp(fileName) == -1) // create a unique name Note that g_mkstemp not only creates a unique name it also opens the file and returns the fd. The file should closed with close (fd). > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:162 > + g_mapped_file_unref(m_mappedFile); We should probably add support for GMappedFile file to GRefPtr

Created attachment 79755 [details] Reverted back to mmap implementation As discussed on IRC, modifications done to the GMappedFile buffer are private to the process which does the modifications. They can't be accessed by another process. So, I reverted back to the mmap implementation. Additionally, usage of g_get_tmp_dir() and closing the fd returned by g_mkstemp are addressed with this patch.

Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation and we will probably use the Win implementation on Windows and the Mac one on Mac OS X and turn the Qt one into a general UNIX one, which could be used by you as well.

(In reply to comment #11) > Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation and we will probably use the Win implementation on Windows and the Mac one on Mac OS X and turn the Qt one into a general UNIX one, which could be used by you as well. You can find it here: https://bugs.webkit.org/show_bug.cgi?id=51984

(In reply to comment #11) > Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation and we will probably use the Win implementation on Windows and the Mac one on Mac OS X and turn the Qt one into a general UNIX one, which could be used by you as well. Kenneth, As I see last patch of https://bugs.webkit.org/show_bug.cgi?id=51984, the patch which you approved r=me, the UNIX implementation was still present in SharedMemoryQt.cpp. Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details?

> Kenneth, > As I see last patch of https://bugs.webkit.org/show_bug.cgi?id=51984, the patch which you approved r=me, the UNIX implementation was still present in SharedMemoryQt.cpp. Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details? Yes, I talked to Kimmo, and he said that it should be little work doing so. I cc'ed him to this bug report, so he might comment on it tomorrow. We might land the patch as it is, and then to the renaming/moving as another step.

(In reply to comment #13) > (In reply to comment #11) > > Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation > Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details? So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following: - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread - Use the ConnectionQt::readyReadHandler as the work queue mainloop - Use select to poll communication socket in the workqueue mainloop - Use additional fd to signal that mainloop should run scheduled work item instead of hanging in select - Use mutexes while appending tasks to work queue

> > So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following: > - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread It might be possible to use the thread abstraction from WTF as well.

(In reply to comment #15) > (In reply to comment #13) > > (In reply to comment #11) > > > Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation > > > Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details? > > > So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following: > - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread > - Use the ConnectionQt::readyReadHandler as the work queue mainloop > - Use select to poll communication socket in the workqueue mainloop > - Use additional fd to signal that mainloop should run scheduled work item instead of hanging in select > - Use mutexes while appending tasks to work queue Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it.

(In reply to comment #17) > > So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following: > > - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread > > - Use the ConnectionQt::readyReadHandler as the work queue mainloop > > - Use select to poll communication socket in the workqueue mainloop > > - Use additional fd to signal that mainloop should run scheduled work item instead of hanging in select > > - Use mutexes while appending tasks to work queue > > Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it. I'm not sure who you're asking, I at least am not working on this at the moment..

(In reply to comment #18) > (In reply to comment #17) > > [...] > > > Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it. > > I'm not sure who you're asking, I at least am not working on this at the moment.. Yep, it was you and Amruth, thanks for commenting. Kphrane told me yesterday in the IRC that Amruth is working on it. Last days I updated all the Minibrowser patches (bug 52805) to compile and work with current HEAD and with gtk2 and gtk3 (I used sharedmemory for gtk3 to avoid gdkpixmaps), so my plan would be to upload all of them with all these changes considering this one is solved (until we know how long it will take to finish the unix solution). With all these patches uploaded we could start working in the gtk webkit2 more easily without blocking, Does that sound good to everybody?

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > > [...] > > > > > Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it. > > > > I'm not sure who you're asking, I at least am not working on this at the moment.. > > Yep, it was you and Amruth, thanks for commenting. Kphrane told me yesterday in the IRC that Amruth is working on it. > > Last days I updated all the Minibrowser patches (bug 52805) to compile and work with current HEAD and with gtk2 and gtk3 (I used sharedmemory for gtk3 to avoid gdkpixmaps), so my plan would be to upload all of them with all these changes considering this one is solved (until we know how long it will take to finish the unix solution). With all these patches uploaded we could start working in the gtk webkit2 more easily without blocking, Does that sound good to everybody? This sounds great. The basic implementation that I've put up here works, but we definitely need to address the drawbacks mentioned in bug 51984, like passing fd's instead of filenames etc. This needs a good amount of code change for files that are already checked-in as well(for eg. Connection class, ProcessLauncher class etc.). Now that you have a patch based on HEAD, it's better we go ahead with all these. Meanwhile I'll create a separate bug for making the patch at 51984 generic and get it accepted by reviewers from both QT and GTK. I'm glad that you've retained the GdkPixmap based solution for GTK2 ;)

(In reply to comment #20) > > [...] > > I'm glad that you've retained the GdkPixmap based solution for GTK2 ;) I didn't I'm sorry :), it added a lot of cruft to the code (with ifdef's) when we can have just one solution for both versions.

Created attachment 82156 [details] Reference patch Just for reference this is the patch I'm using for testing the other patches, I did not review it, just little modifications. I upload it here so other people can test the other patches meanwhile Amruth finishes the unix solution.

Amruth do you have any new version of this patch already?

Created attachment 84965 [details] Shared Memory changes Alexg, I have a basic version of the patch which pulls in required changes from Qt. I verified these on r80210. There is still some cleanup pending and a few cases to be considered ie. to handle process termination, renaming the files and more testing. I wasn't able to do more cleanup as i am stuck with some other deliverables which are taking all my time. By the way, I intentionally did not rename the files *Qt.cpp to *Unix.cpp so as to review the delta changes easily. Had I renamed them, it would have been difficult to review the changes since it would become a new file.

(In reply to comment #24) > Created an attachment (id=84965) [details] > Shared Memory changes > > Alexg, > I have a basic version of the patch which pulls in required changes from Qt. I verified these on r80210. There is still some cleanup pending and a few cases to be considered ie. to handle process termination, renaming the files and more testing. I wasn't able to do more cleanup as i am stuck with some other deliverables which are taking all my time. > > By the way, I intentionally did not rename the files *Qt.cpp to *Unix.cpp so as to review the delta changes easily. Had I renamed them, it would have been difficult to review the changes since it would become a new file. Thanks for information Amruth :), I'm going to test and check the patch.

Created attachment 85553 [details] Dusted up version of Amruth's patch with a ChangeLog

Created attachment 85555 [details] Previous patch with file movement intact webkit-patch did not seem to record the file movement, so I've attached a patch I created manually.

Comment on attachment 85555 [details] Previous patch with file movement intact View in context: https://bugs.webkit.org/attachment.cgi?id=85555&action=review I guess you should remove ConnectionGtk.cpp too. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:105 > +void Connection::platformInvalidate() > +{ You should call m_connectionQueue.unregisterEventSourceHandler(m_socketDescriptor); for GTK platform here before closing the socket

Created attachment 87395 [details] Patch with Carlos' suggestions

(In reply to comment #28) > (From update of attachment 85555 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85555&action=review > > I guess you should remove ConnectionGtk.cpp too. > > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:105 > > +void Connection::platformInvalidate() > > +{ > > You should call m_connectionQueue.unregisterEventSourceHandler(m_socketDescriptor); for GTK platform here before closing the socket Thanks for the review Carlos. I've uploaded a new patch fixing these issues.

Attachment 87395 [details] did not build on qt: Build output: http://queues.webkit.org/results/8282535

Good to see that you have been successfully merged the two implementation. Please, fix the Qt build and go ahead with the patch! ;)

After taking a look at #57506 (that looks like a duplicate) I realized that we should think over the terminology. Actually the API's we use are all POSIX so it can be a better naming convention then UNIX. I think file names should follow the style guideline so acronyms should be capitalized (FooPOSIX.cpp instead of FooPosix.cpp). This is how the JIT files are named for example. Maybe a new macro flag would be useful to replace the "PLATFORM(QT) || PLATFORM(GTK)" conditions with smg like USE(POSIX_IPC).

*** Bug 57506 has been marked as a duplicate of this bug. ***

(In reply to comment #33) > After taking a look at #57506 (that looks like a duplicate) I realized that we should think over the terminology. Actually the API's we use are all POSIX so it can be a better naming convention then UNIX. I think file names should follow the style guideline so acronyms should be capitalized (FooPOSIX.cpp instead of FooPosix.cpp). This is how the JIT files are named for example. Maybe a new macro flag would be useful to replace the "PLATFORM(QT) || PLATFORM(GTK)" conditions with smg like USE(POSIX_IPC). I'm not sure that POSIX is the appropriate terminology for all of these files. ConnectionUnix makes heavy use of ancillary data structures (cmsghdr) to pass file handles through sockets. As far as I know this is not defined in POSIX at all. Perhaps we could simply have a greater distinction: AttachmentPOSIX.cpp SharedMemoryPOSIX.cpp ConnectionUnix.cpp How does that sound?

(In reply to comment #35) > (In reply to comment #33) > > After taking a look at #57506 (that looks like a duplicate) I realized that we should think over the terminology. Actually the API's we use are all POSIX so it can be a better naming convention then UNIX. I think file names should follow the style guideline so acronyms should be capitalized (FooPOSIX.cpp instead of FooPosix.cpp). This is how the JIT files are named for example. Maybe a new macro flag would be useful to replace the "PLATFORM(QT) || PLATFORM(GTK)" conditions with smg like USE(POSIX_IPC). > > I'm not sure that POSIX is the appropriate terminology for all of these files. ConnectionUnix makes heavy use of ancillary data structures (cmsghdr) to pass file handles through sockets. As far as I know this is not defined in POSIX at all. Perhaps we could simply have a greater distinction: > > AttachmentPOSIX.cpp > SharedMemoryPOSIX.cpp > ConnectionUnix.cpp > > How does that sound? Correct, I was wrong about POSIX. This makes it more complicated. In this case I would better like to just use UNIX everywhere since POSIX is part of it. In that case only the capitalization should be changed in your patch. But your new proposal is also consistent so we can chose it.

Comment on attachment 87395 [details] Patch with Carlos' suggestions It seems you forgot to add the new files, and remove the Qt ones.

Created attachment 87737 [details] Patch with all files Ah, bitten again by that webkit_patch bug! I've attached the correct patch.

Comment on attachment 87737 [details] Patch with all files View in context: https://bugs.webkit.org/attachment.cgi?id=87737&action=review > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:122 > +#if PLATFORM(GTK) > + m_connectionQueue.unregisterEventSourceHandler(m_socketDescriptor); > +#endif This should be called before closing the socket.

Created attachment 87745 [details] Patch unregistering event source handler before setting the socket descriptor to -1 Nice catch Carlos! I've updated the patch to unregister the event source *before* setting m_socketDescriptor to -1.

Comment on attachment 87745 [details] Patch unregistering event source handler before setting the socket descriptor to -1 looks ok to me

Comment on attachment 87745 [details] Patch unregistering event source handler before setting the socket descriptor to -1 This looks pretty good!

Comment on attachment 87745 [details] Patch unregistering event source handler before setting the socket descriptor to -1 View in context: https://bugs.webkit.org/attachment.cgi?id=87745&action=review > Source/WebKit2/ChangeLog:20 > + * Platform/CoreIPC/unix/AttachmentUnix.cpp: Renamed from Source/WebKit2/Platform/CoreIPC/qt/AttachmentQt.cpp. The Changelog is out of date. The file you add is AttachmentPOSIX.cpp. Please fix this before landing. > Source/WebKit2/GNUmakefile.am:100 > + Source/WebKit2/Platform/CoreIPC/posix/AttachmentPOSIX.cpp \ I'm not a fan of adding two new terminology with new dirs and file name patterns. (Just my personal opinion, should not block the patch).

(In reply to comment #43) > > Source/WebKit2/GNUmakefile.am:100 > > + Source/WebKit2/Platform/CoreIPC/posix/AttachmentPOSIX.cpp \ > > I'm not a fan of adding two new terminology with new dirs and file name patterns. (Just my personal opinion, should not block the patch). I prefer using "Unix" everywhere as well. Kenneth, does your review still hold with that change?

(In reply to comment #44) > (In reply to comment #43) > > > > Source/WebKit2/GNUmakefile.am:100 > > > + Source/WebKit2/Platform/CoreIPC/posix/AttachmentPOSIX.cpp \ > > > > I'm not a fan of adding two new terminology with new dirs and file name patterns. (Just my personal opinion, should not block the patch). > > I prefer using "Unix" everywhere as well. Kenneth, does your review still hold with that change? Yes! :-)

Committed r83215: <http://trac.webkit.org/changeset/83215>