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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1173051.patch, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26bda1353dc8

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1173051.patch, v1.1

Remove whitespace only change that sneaked in.

Comment 3

[Dave Hylands [: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.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1173051.patch, v2

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.

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 8617712 [details] [diff] [review]
bug1173051.patch, v2

Review of attachment 8617712 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/62896b7f8e2f

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62896b7f8e2f