Closed Bug 1173051 Opened 6 years ago Closed 6 years ago

[DeviceStorage] Move filtering of enumeration results from child to parent

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

Details

Attachments

(1 file, 2 obsolete files)

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: nobody → aosmond
Status: NEW → ASSIGNED
Attached patch bug1173051.patch, v1.1 (obsolete) — Splinter Review
Remove whitespace only change that sneaked in.
Attachment #8617498 - Attachment is obsolete: true
Attachment #8617502 - Flags: review?(dhylands)
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+
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 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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.