Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9030031:
Move InMemoryURLIndex Caching Operations to FILE Thread (Closed)
|
Created:
8 years, 11 months ago by mrossetti Modified:
8 years, 9 months ago Reviewers:
brettw CC:
chromium-reviews, ncarter (slow), akalin, Raghu Simha, brettw-cc_chromium.org, James Su, tim (not reviewing) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMove InMemoryURLIndex Caching Operations to FILE Thread
Reading and writing of the InMemoryURLIndex (IMUI) cache is now peformed on the FILE thread and, when required, rebuilding of the index is performed on the history thread. Reading and rebuilding of the index from the cache file or history database are now performed as atomic operations rather than by restoring/rebuilding in-place, i.e. a new private data object is created and populated then swapped in for the live private data object.
BUG=83659
TEST=Unit tests updated/enhanced.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126982
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : Fix linux private/friend error. #Patch Set 4 : Sync hoping to clear up mac_sync builder fails in sync_integration_tests #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : Missed a private data swap. Try to fix update phase failure. #
Total comments: 58
Patch Set 8 : #Patch Set 9 : Pass bool value, not pointer. Sync to clear up Linux fails (hopefully). #
Total comments: 6
Patch Set 10 : Use proper refcounted structures for passing around status. #
Total comments: 10
Patch Set 11 : #
Total comments: 14
Patch Set 12 : #Patch Set 13 : Fully embrace scoped_refptr for private_data. #Patch Set 14 : Added missing changed file to CL. #
Total comments: 6
Patch Set 15 : Return scoped_refptr. #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : Syncing with hopes of pleasing trybot update #
Messages
Total messages: 16 (0 generated)
Peter, this is ready for review. The win_sync failure is a flake. This is the final step in moving the cache file read/write to the FILE thread and any rebuilding of the index to the history thread. It was necessary to move ownership of the InMemoryURLIndex from the InMemoryHistoryBackend to the HistoryBackend in order to accomplish this as the IMUI now relies on notifications for updating the index. Once this is in I have the (easy) fix for http://crbug.com/102957 (Google Docs document titles not displaying from Omnibox suggestions) and probably also http://crbug.com/87803 (HQP: Title-based results need session restart.) but this easy fix relies on the changes in _this_ CL.
http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/hist... File chrome/browser/history/history.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/hist... chrome/browser/history/history.cc:186: InMemoryIndex()->ShutDown(); Nit: Should this instead access |history_backend_| directly, to avoid the LoadBackendIfNecessary() call? http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:42: return true; Idle curiosity... what would be the consequences of just never notifying the IMUI on failure? http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:60: // TODO(mrossetti): Register for profile/language change notifications. What is a profile change notification? We don't ever change the profile for a browser object. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:126: BrowserThread::FILE, FROM_HERE, Nit: You could put these on the above line if you want. (Several places) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:188: private_data_->Clear(); Nit: Would it make sense to allow |private_data_| to be NULL when there is no private data? (This would mean we wouldn't need to create an empty object in the IMUI initializer list, and we could eliminate the "else" clause and the bool here if |private_data| is NULL on failure. Actually we could probably do the latter regardless.) Or would that mean that if restoration fails then we will not overwrite the disk file with whatever new data we collect this run? (Will we do that currently?) This also makes me wonder if we have some sort of dataloss of navigations that happen while we're busy restoring the cache. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:195: void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB( This function seems basically identical to OnCacheRestored(). One deletes its arg and one doesn't, which seems odd (is that a bug?). Also they arbitrarily have different result ordering. Can we combine these? http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:244: private_data_->UpdateURL(details->row); Nit: Should we be calling IMUI::UpdateURL() or DeleteURL() instead in these next three functions? (I don't know if that's in any way clearer or more future-proof if you want to make updating more complex) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:245: needs_to_be_cached_ = true; Nit: Does this need to be after the above line? Doing it before would be consistent with the next two functions. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:250: for (std::vector<history::URLRow>::const_iterator row = Nit: Is there a typedef somewhere (history.h? I dunno) for vector<URLRow>? Should there be? (2 places) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.h (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:61: class InMemoryURLIndex : public base::RefCountedThreadSafe<InMemoryURLIndex>, We chatted some in IM about avoiding this by vending weak pointers and deleting on the UI thread. It might be possible to use the existing |shutdown_| member to prevent any indirect access to other threads, if you elect to go the DeleteSoon() route to delete this on the UI thread. Alternatively it might be possible to use the call you've added to HistoryService::UnloadBackend() to delete this object directly on the UI thread, if we can then prevent the object from getting recreated. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:84: virtual void OnCacheSaveFinished() = 0; // Is called on the IO thread. Nit: Since this is subtle, it might make sense to put "IOThread" at the end of the function name rather than in a comment, a la the HistoryDBTask functions. (2 places) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:131: FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, AddNewRows); Nit: Does it make sense to build some base class for all the InMemoryURLIndexTests, give it a bunch of passthrough accessors, and then just friend that class here? http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:151: // We take ownership of the |private_data|. Nit: |private_data| is not a variable defined here (or, by this name, below) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:153: Nit: No newline http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:156: virtual bool RunOnDBThread(HistoryBackend* backend, Nit: Optional: "// HistoryDBTask" http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:158: Nit: Newline unnecessary http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:175: // Construct a file path for the cache file within the same directory where Nit: Construct -> Constructs (while you're here) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:185: // Restores private_data_ from the given |path|. Runs on the UI thread. Nit: You might want an explicit comment here or above clarifying what the difference between these two functions is. (2 places) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:200: // Called by DoSaveToCacheFile and saves |private_data| to the given |path|. Nit: and saves -> to save http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:212: // instanceof the private data just rebuilt. Nit: Missing space http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:221: // Notifications ------------------------------------------------------------- Nit: Kinda weird to have ---- here but not above (e.g. on "// Utility functions ...", and no divider below (above RebuildFromHistory() or something). http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:248: // Observers to notify upon restoral or save of the private data cache. Nit: restoral or save -> restoration or saving http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:254: // Remember to unregister from notifications on destruction. Nit: Comment unnecessary http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_unittest.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_unittest.cc:46: explicit CacheFileSaverObserver(MessageLoop* loop) Nit: I know it's just a test file, but I'd prefer not to inline constructors or virtual functions all the same. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... File chrome/browser/history/url_database.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_database.cc:266: return false; We access GetDB() directly a lot in the rest of this file. Is there a reason this case should differ? http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.cc:89: // presented to the user). Nit: Honestly, the old comment seemed a lot clearer to me... http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.cc:342: bool operator()(const std::pair<const HistoryID, URLRow> item) { Nit: Missing &? http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.cc:1026: URLRow row; Nit: You can condense these three lines to two and get |row| scoped to the loop as a bonus: for (URLRow row; history_enum.GetNextURL(&row); ) rebuilt_data->IndexRow(row); http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.h (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:46: #endif This seems like it should have disappeared before uploading...
This is my first effort at introducing some threading concepts into Chrome so I would ask sky/brettw (arm wrestle for who does the review :) to concentrate on that aspect of this change. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/hist... File chrome/browser/history/history.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/hist... chrome/browser/history/history.cc:186: InMemoryIndex()->ShutDown(); On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Should this instead access |history_backend_| directly, to avoid the > LoadBackendIfNecessary() call? Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:42: return true; On 2012/01/14 00:12:49, Peter Kasting wrote: > Idle curiosity... what would be the consequences of just never notifying the > IMUI on failure? The index data would still have been cleared so there are no ramifications. So in the following DoneRunOnMainThread() function I'll just not call the DoneRebuidingPrivateDataFromHistoryDB(...) function and simplify the implementation thereof. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:60: // TODO(mrossetti): Register for profile/language change notifications. On 2012/01/14 00:12:49, Peter Kasting wrote: > What is a profile change notification? We don't ever change the profile for a > browser object. Comment changes to only refer to language changes. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:126: BrowserThread::FILE, FROM_HERE, On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: You could put these on the above line if you want. (Several places) This part of the change has been postponed. I'll address this in the next CL where I add the FILE/pool handling. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:188: private_data_->Clear(); On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Would it make sense to allow |private_data_| to be NULL when there is no > private data? (This would mean we wouldn't need to create an empty object in > the IMUI initializer list, and we could eliminate the "else" clause and the bool > here if |private_data| is NULL on failure. Actually we could probably do the > latter regardless.) Or would that mean that if restoration fails then we will > not overwrite the disk file with whatever new data we collect this run? (Will > we do that currently?) This also makes me wonder if we have some sort of > dataloss of navigations that happen while we're busy restoring the cache. I've been thinking about this actually. The approach I was considering (for a future CL) was to stack up the visit/delete notifications (that are being implemented in this CL) while the cache was being restored. We could certainly trigger the need to do so off of the |private_data_| being NULL. In the case of failure to restore from the cache we could leave |private_data_| null but then if the successive attempt to rebuild from the history database failed we'd need to have a |private_data_| so that we can start collecting visits afresh. This would be a pretty serious failure. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:195: void InMemoryURLIndex::DoneRebuidingPrivateDataFromHistoryDB( On 2012/01/14 00:12:49, Peter Kasting wrote: > This function seems basically identical to OnCacheRestored(). One deletes its > arg and one doesn't, which seems odd (is that a bug?). Also they arbitrarily > have different result ordering. Can we combine these? Possibly. I'll consider this for the next change as the FILE/pool work is being dealt with in a separate CL. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:244: private_data_->UpdateURL(details->row); On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Should we be calling IMUI::UpdateURL() or DeleteURL() instead in these next > three functions? (I don't know if that's in any way clearer or more > future-proof if you want to make updating more complex) Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:245: needs_to_be_cached_ = true; On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Does this need to be after the above line? Doing it before would be > consistent with the next two functions. I'll move them to the bottom for now for consistency. I'm also going to have the UpdateURL and DeleteURL functions return a bool to indicate if there was an actual change. If there was then needs_to_be_cached_ will be set to true. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:250: for (std::vector<history::URLRow>::const_iterator row = On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Is there a typedef somewhere (history.h? I dunno) for vector<URLRow>? > Should there be? (2 places) Done. Added 'URLRows' to history_types.h. Will file a separate CL for changing all of the unrelated-to-this-CL uses. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.h (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:61: class InMemoryURLIndex : public base::RefCountedThreadSafe<InMemoryURLIndex>, On 2012/01/14 00:12:49, Peter Kasting wrote: > We chatted some in IM about avoiding this by vending weak pointers and deleting > on the UI thread. > > It might be possible to use the existing |shutdown_| member to prevent any > indirect access to other threads, if you elect to go the DeleteSoon() route to > delete this on the UI thread. Alternatively it might be possible to use the > call you've added to HistoryService::UnloadBackend() to delete this object > directly on the UI thread, if we can then prevent the object from getting > recreated. Done. Notifications are now employed. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:84: virtual void OnCacheSaveFinished() = 0; // Is called on the IO thread. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Since this is subtle, it might make sense to put "IOThread" at the end of > the function name rather than in a comment, a la the HistoryDBTask functions. > (2 places) This has been postponed to the next CL when the FILE/resource pool is employed. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:131: FRIEND_TEST_ALL_PREFIXES(InMemoryURLIndexTest, AddNewRows); On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Does it make sense to build some base class for all the > InMemoryURLIndexTests, give it a bunch of passthrough accessors, and then just > friend that class here? Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:151: // We take ownership of the |private_data|. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: |private_data| is not a variable defined here (or, by this name, below) Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:153: On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: No newline Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:158: On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Newline unnecessary Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:175: // Construct a file path for the cache file within the same directory where On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Construct -> Constructs (while you're here) Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:185: // Restores private_data_ from the given |path|. Runs on the UI thread. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: You might want an explicit comment here or above clarifying what the > difference between these two functions is. (2 places) Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:200: // Called by DoSaveToCacheFile and saves |private_data| to the given |path|. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: and saves -> to save Postponed to the FILE/pool change. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:212: // instanceof the private data just rebuilt. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Missing space Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:221: // Notifications ------------------------------------------------------------- On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Kinda weird to have ---- here but not above (e.g. on "// Utility functions > ...", and no divider below (above RebuildFromHistory() or something). Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:248: // Observers to notify upon restoral or save of the private data cache. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: restoral or save -> restoration or saving Postponed to the FILE/pool change. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:254: // Remember to unregister from notifications on destruction. On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Comment unnecessary Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_unittest.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_unittest.cc:46: explicit CacheFileSaverObserver(MessageLoop* loop) On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: I know it's just a test file, but I'd prefer not to inline constructors or > virtual functions all the same. Will do in the next CL. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... File chrome/browser/history/url_database.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_database.cc:266: return false; On 2012/01/14 00:12:49, Peter Kasting wrote: > We access GetDB() directly a lot in the rest of this file. Is there a reason > this case should differ? Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.cc:89: // presented to the user). On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Honestly, the old comment seemed a lot clearer to me... Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.cc:342: bool operator()(const std::pair<const HistoryID, URLRow> item) { On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: Missing &? Done. http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.cc:1026: URLRow row; On 2012/01/14 00:12:49, Peter Kasting wrote: > Nit: You can condense these three lines to two and get |row| scoped to the loop > as a bonus: > > for (URLRow row; history_enum.GetNextURL(&row); ) > rebuilt_data->IndexRow(row); w00t! My ancient mind just doesn't work that way! ;^) http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.h (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:46: #endif On 2012/01/14 00:12:49, Peter Kasting wrote: > This seems like it should have disappeared before uploading... Done.
http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/32023/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:126: BrowserThread::FILE, FROM_HERE, It's not clear what you mean here? What part has been postponed? http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:268: private_data_copy.release())); You do get() and release() on the same var. I don't think the spec defines what order the args get evaluated as, so this may do the wrong thing. Also, this will leak if the browser exits while you're executing, since you won't delete the data, which will turn the memory bots red. Actually, it's not clear why you need to pass the data back to the UI thread in the first place since all you do is delete it. You should be able to pass a scoped_ptr as the arg (see base/bind_helpers.h and search for scoped_ptr for an example). http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:277: void InMemoryURLIndex::OnCacheSaveDone(bool* succeeded, It looks like you leak this bool, and this will have the same problems as the "copy" assuming you did. Maybe what you need here is to create a refcounted threadsafe object containing both of these items (bool and data) and pass scoped_refptrs as the arguments. This will keep it alive for the duration of your calls and automatically clean everything up when you're done or if the browser shuts down in the middle. http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.h (right): http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:209: static void DeleteCacheFile(const FilePath path); Pass by ref.
Mike, since this change kind of sat for seven weeks and now has become smaller, can you ensure the change title/description are up to date and then just quickly restate who you want to look at what? I'm not sure if you still want me reviewing the whole thing, or if this change now only covers bits you want Brett and Scott to look at, or what.
-pkasting, -sky Changed how parms are passed through the closures. http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:268: private_data_copy.release())); Thanks! That's exactly the kind of guidance I needed! I'm now using a scoped_refptr for passing the copy of the private data around. This eliminates the need to pass the copy to the completion routine for deletion. I created a RefCountedBool for passing around the success status. Please let me know if this is appropriate or too heavy for such a simple status flag. On 2012/03/05 22:13:05, brettw wrote: > You do get() and release() on the same var. I don't think the spec defines what > order the args get evaluated as, so this may do the wrong thing. > > Also, this will leak if the browser exits while you're executing, since you > won't delete the data, which will turn the memory bots red. Actually, it's not > clear why you need to pass the data back to the UI thread in the first place > since all you do is delete it. You should be able to pass a scoped_ptr as the > arg (see base/bind_helpers.h and search for scoped_ptr for an example). http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:277: void InMemoryURLIndex::OnCacheSaveDone(bool* succeeded, See above. I'm now using a ref-counted bool container class. On 2012/03/05 22:13:05, brettw wrote: > It looks like you leak this bool, and this will have the same problems as the > "copy" assuming you did. Maybe what you need here is to create a refcounted > threadsafe object containing both of these items (bool and data) and pass > scoped_refptrs as the arguments. This will keep it alive for the duration of > your calls and automatically clean everything up when you're done or if the > browser shuts down in the middle. http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.h (right): http://codereview.chromium.org/9030031/diff/46001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:209: static void DeleteCacheFile(const FilePath path); On 2012/03/05 22:13:05, brettw wrote: > Pass by ref. Done. http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:239: PostSaveToCacheFileTask(); // Cache the newly rebuilt index. I'll fix the lint complaint about needing two spaces before the comment before committing. http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_types.cc (right): http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.cc:157: return sizeof(bool); I will change this 'bool' to the var 'bool_' before I commit.
Ready for review.
http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:52: index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release()); Style: outdent 2 sp http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:180: URLIndexPrivateData* restored_private_data = NULL; Unless I'm missing something obvious, this code doesn't work at all. Have you tested this patch? http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:266: new URLIndexPrivateData(*(private_data))); Style: normally we wouldn't put parens after the * here. http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.h (right): http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:207: static void DeleteCacheFile(const FilePath& path); I'd probably make this a file static since you don't need it outside of the .cc file so this header is smaller.
I fixed that glaring issue! Ready for another review. http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:52: index_->DoneRebuidingPrivateDataFromHistoryDB(succeeded_, data_.release()); On 2012/03/08 22:03:54, brettw wrote: > Style: outdent 2 sp Done. http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:180: URLIndexPrivateData* restored_private_data = NULL; Yes, but I'm sure it passes the unit test only as a fluke. :^[ Fixed by using a special scoped_refptr class. On 2012/03/08 22:03:54, brettw wrote: > Unless I'm missing something obvious, this code doesn't work at all. Have you > tested this patch? http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:266: new URLIndexPrivateData(*(private_data))); On 2012/03/08 22:03:54, brettw wrote: > Style: normally we wouldn't put parens after the * here. Done. http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.h (right): http://codereview.chromium.org/9030031/diff/53001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.h:207: static void DeleteCacheFile(const FilePath& path); On 2012/03/08 22:03:54, brettw wrote: > I'd probably make this a file static since you don't need it outside of the .cc > file so this header is smaller. Done.
http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:227: private_data_->Clear(); You leak the data here. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_types.h (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:82: class RefCountedBool : public RefCountedMemory { IT wasn't the intent for all random types to go in this file, especially since this is an internal type used by only one function. I think this should be a member of the class that needs it. You can probably forward declare it to keep everything but the forward declare in the .cc file. Don't derive from RefCountedMemory, that's for a bag of bytes. Just derive from RefCounted. If you're passing between threads (I think you are), you need to use RefCountedThreadSafe http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:86: virtual const unsigned char* front() const OVERRIDE; Delete these. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:89: bool value() { return bool_; } This should be const. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:96: bool bool_; This should be called "value_" to match the getter & setter http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.h (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:39: URLIndexPrivateData* release(); Delete release and reset. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:46: scoped_ptr<URLIndexPrivateData> data_; I don't think you should wrap a scoped_ptr with a reference counted object. Why not just make your object refcounted?
I'll just get rid of that wacky thread safe container class and add a copy function to the private data class. That will resolve the rest of your issues. I'd originally wanted the private data class to be copy-able. But I can work around that. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:227: private_data_->Clear(); Fixed. On 2012/03/12 04:37:40, brettw wrote: > You leak the data here. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.h (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:46: scoped_ptr<URLIndexPrivateData> data_; Because I wanted the original class to be copy-able. No problem, though, I'll get rid of this wacky container class and just write my own copy function. On 2012/03/12 04:37:40, brettw wrote: > I don't think you should wrap a scoped_ptr with a reference counted object. Why > not just make your object refcounted?
I've gotten rid of the funky scoped_refptr class containing a scoped_ptr and made the class being passed around RefCountedThreadSafe. This necessitated a little refactoring and revealed a bug that I fixed. I also moved a couple of data members from the URLIndexPrivateData class back up into the InMemoryURLIndex class (language_ and scheme_white_list_) as it was the more appropriate place for them. Ready to review whenever it's convenient. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_types.h (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:82: class RefCountedBool : public RefCountedMemory { On 2012/03/12 04:37:40, brettw wrote: > IT wasn't the intent for all random types to go in this file, especially since > this is an internal type used by only one function. This class is used to exchange status between the InMemoryURLIndex class and the URLIndexPrivateData class so I moved it into in_memory_url_index.h but not as a member of the InMemoryURLIndex class since subclasses cannot be forward declared. > I think this should be a member of the class that needs it. You can probably > forward declare it to keep everything but the forward declare in the .cc file. > > Don't derive from RefCountedMemory, that's for a bag of bytes. Just derive from > RefCounted. If you're passing between threads (I think you are), you need to use > RefCountedThreadSafe Done. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:86: virtual const unsigned char* front() const OVERRIDE; On 2012/03/12 04:37:40, brettw wrote: > Delete these. Done. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:89: bool value() { return bool_; } On 2012/03/12 04:37:40, brettw wrote: > This should be const. Done. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... File chrome/browser/history/url_index_private_data.h (right): http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:39: URLIndexPrivateData* release(); On 2012/03/12 04:37:40, brettw wrote: > Delete release and reset. This whole class is gone now. http://codereview.chromium.org/9030031/diff/60001/chrome/browser/history/url_... chrome/browser/history/url_index_private_data.h:46: scoped_ptr<URLIndexPrivateData> data_; On 2012/03/12 04:37:40, brettw wrote: > I don't think you should wrap a scoped_ptr with a reference counted object. Why > not just make your object refcounted? Done.
http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:230: else Style nit: need {} for this if/else block since you have >1 line in one of the arms. http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:282: scoped_refptr<URLIndexPrivateData> private_data_copy = Doesn't this leak a reference? Duplicate is returning you a ref, and this scoped_refptr will take another one. I think it will be best to return a scoped_refptr. http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_types.h (right): http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:13: #include "base/memory/ref_counted.h" This may be unnecessary, remove it if so.
Updated! http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index.cc (right): http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:230: else On 2012/03/13 23:33:52, brettw wrote: > Style nit: need {} for this if/else block since you have >1 line in one of the > arms. Done. http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index.cc:282: scoped_refptr<URLIndexPrivateData> private_data_copy = Gah! Of course, you're right about the leak. I'll get it, eventually. ;^) On 2012/03/13 23:33:52, brettw wrote: > Doesn't this leak a reference? Duplicate is returning you a ref, and this > scoped_refptr will take another one. > > I think it will be best to return a scoped_refptr. http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... File chrome/browser/history/in_memory_url_index_types.h (right): http://codereview.chromium.org/9030031/diff/69020/chrome/browser/history/in_m... chrome/browser/history/in_memory_url_index_types.h:13: #include "base/memory/ref_counted.h" On 2012/03/13 23:33:52, brettw wrote: > This may be unnecessary, remove it if so. Done.
lgtm
Thanks for the review and the training. I think I have a much better understanding now of the usage of the various thread safety support mechanisms. Some final integration changes were required as some other modifications had been made to the url_index_private_data.h/.cc that had to be accommodated. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
