Closed Bug 801376 Opened 7 years ago Closed 6 years ago

[OS.File] Make loading symbols lazy

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Whiteboard: [Async][mentor=Yoric][lang=js][memshrink])

Attachments

(1 file, 4 obsolete files)

At the moment, all the symbols are loaded eagerly from libc. However, there are good chances that 
1/ most symbols will not be used during the execution of OS.File on a worker thread;
2/ even the "official" file I/O thread that backs off-main-thread IO will certainly not use most of these symbols until rather late in the execution of the process.

We should therefore make sure that symbols are loaded lazily.
Whiteboard: [Async]
This should help bring memory use of OS.File down.
Whiteboard: [Async] → [Async][mentor=Yoric][lang=js][memshrink]
Assignee: nobody → dteller
Attached patch Loading symbols lazily, v2 (obsolete) — Splinter Review
Changes from v1:
- added missing documentation;
- made sure that all foreign functions are now defined lazily.
Attachment #818732 - Attachment is obsolete: true
Attachment #818747 - Flags: review?(nfroyd)
Comment on attachment 818747 [details] [diff] [review]
Loading symbols lazily, v2

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

Have you checked to see whether this affects memory use/startup time?  I can see OS.File initialization taking up time on Android pageload profiles...

::: toolkit/components/osfile/modules/osfile_unix_back.jsm
@@ +37,5 @@
>           declareFFI = aDeclareFFI.bind(null, libc);
>         } else {
>           declareFFI = SysAll.declareFFI;
>         }
> +       let declareLazyFFI = SharedAll.declareLazyFFI;

You could do |let declareLazy = ...| for consistency.  Optional.
Attachment #818747 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 818747 [details] [diff] [review]
> Loading symbols lazily, v2
> 
> Review of attachment 818747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have you checked to see whether this affects memory use/startup time?  I can
> see OS.File initialization taking up time on Android pageload profiles...

No, I haven't checked yet.
I believe that it should improve both memory use and startup time, because we probably use only few of the primitives during startup.
Attached patch Loading symbols lazily, v3 (obsolete) — Splinter Review
Fixed a symbol that was defined twice, causing errors under Linux.
Attachment #818747 - Attachment is obsolete: true
Attachment #819231 - Flags: review+
Attached patch Loading symbols lazily, v4 (obsolete) — Splinter Review
This time, with the patch.
Also, fixed typos that caused Windows tests to fail.
Try: https://tbpl.mozilla.org/?tree=Try&rev=f533d4590e92
Attachment #819231 - Attachment is obsolete: true
Attachment #819236 - Flags: review+
Once more, with feeling.
Try: https://tbpl.mozilla.org/?tree=Try&rev=5a64dfb442fa
Attachment #819236 - Attachment is obsolete: true
Attachment #820231 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b9c7369eb615
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 928385
You need to log in before you can comment on or make changes to this bug.