Closed
Bug 1173051
Opened 8 years ago
Closed 8 years ago
[DeviceStorage] Move filtering of enumeration results from child to parent
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: aosmond, Assigned: aosmond)
Details
Attachments
(1 file, 2 obsolete files)
4.82 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
Currently the child process filters the enumeration results in ContinueCursorEvent::GetNextFile() to ensure that only files matching the extensions masks are passed to the caller. This check should be moved to DeviceStorageFile::collectFilesInternal so that the parent does the filtering. This has the advantage of reducing the IPC overhead.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26bda1353dc8
Assignee | ||
Comment 2•8 years ago
|
||
Remove whitespace only change that sneaked in.
Attachment #8617498 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8617502 -
Flags: review?(dhylands)
Comment 3•8 years ago
|
||
Comment on attachment 8617502 [details] [diff] [review] bug1173051.patch, v1.1 Review of attachment 8617502 [details] [diff] [review]: ----------------------------------------------------------------- I think this is a definite improvement. I just had one further refinement I think could be made. ::: dom/devicestorage/DeviceStorageRequestChild.cpp @@ +192,5 @@ > nsDOMDeviceStorageCursor* cursor > = static_cast<nsDOMDeviceStorageCursor*>(mRequest.get()); > > uint32_t count = r.paths().Length(); > + cursor->mFiles.SetCapacity(count); nice ::: dom/devicestorage/nsDeviceStorage.cpp @@ +2045,5 @@ > return nullptr; > } > > + nsRefPtr<DeviceStorageFile> file = cursor->mFiles[0].forget(); > + cursor->mFiles.RemoveElementAt(0); This still feels extremely inefficient. With a large list, this is going to move the remaining items down a slot. It is just an array of pointers. But if there are 10,000 files in the array, then this will move 9999 pointers down a slot, and then move 9998 pointers down a slot, etc. I think you should add an index to the cursor, and set each entry to nullptr as we return them (so we still free the memory as we iterate), but that would make iterating through large lists much faster.
Attachment #8617502 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 4•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8151d98e34a (In reply to Dave Hylands [:dhylands] from comment #3) > Comment on attachment 8617502 [details] [diff] [review] > bug1173051.patch, v1.1 > > Review of attachment 8617502 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this is a definite improvement. > > I just had one further refinement I think could be made. > > ::: dom/devicestorage/DeviceStorageRequestChild.cpp > @@ +192,5 @@ > > nsDOMDeviceStorageCursor* cursor > > = static_cast<nsDOMDeviceStorageCursor*>(mRequest.get()); > > > > uint32_t count = r.paths().Length(); > > + cursor->mFiles.SetCapacity(count); > > nice > > ::: dom/devicestorage/nsDeviceStorage.cpp > @@ +2045,5 @@ > > return nullptr; > > } > > > > + nsRefPtr<DeviceStorageFile> file = cursor->mFiles[0].forget(); > > + cursor->mFiles.RemoveElementAt(0); > > This still feels extremely inefficient. With a large list, this is going to > move the remaining items down a slot. > > It is just an array of pointers. But if there are 10,000 files in the array, > then this will move 9999 pointers down a slot, and then move 9998 pointers > down a slot, etc. > > I think you should add an index to the cursor, and set each entry to nullptr > as we return them (so we still free the memory as we iterate), but that > would make iterating through large lists much faster. Good idea, I updated the patch to do just that.
Attachment #8617502 -
Attachment is obsolete: true
Attachment #8617712 -
Flags: review?(dhylands)
Comment 5•8 years ago
|
||
Comment on attachment 8617712 [details] [diff] [review] bug1173051.patch, v2 Review of attachment 8617712 [details] [diff] [review]: ----------------------------------------------------------------- Excellent.
Attachment #8617712 -
Flags: review?(dhylands) → review+
https://hg.mozilla.org/mozilla-central/rev/62896b7f8e2f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•