Closed
Bug 801376
Opened 8 years ago
Closed 7 years ago
[OS.File] Make loading symbols lazy
Categories
(Toolkit :: OS.File, defect)
Toolkit
OS.File
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
(Whiteboard: [Async][mentor=Yoric][lang=js][memshrink])
Attachments
(1 file, 4 obsolete files)
40.89 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [Async]
Assignee | ||
Comment 1•7 years ago
|
||
This should help bring memory use of OS.File down.
Whiteboard: [Async] → [Async][mentor=Yoric][lang=js][memshrink]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dteller
Assignee | ||
Comment 2•7 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=3c528d16ddf7
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
Fixed a symbol that was defined twice, causing errors under Linux.
Attachment #818747 -
Attachment is obsolete: true
Attachment #819231 -
Flags: review+
Assignee | ||
Comment 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
Once more, with feeling. Try: https://tbpl.mozilla.org/?tree=Try&rev=5a64dfb442fa
Attachment #819236 -
Attachment is obsolete: true
Attachment #820231 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c7369eb615
Keywords: checkin-needed
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b9c7369eb615
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•