DNR: Decide whether to the ruleset file should be mmap-ed. |
|||
Issue descriptionIt needs to be decided whether the flatbuffer ruleset file should be mmap-ed or not. Alternative 1: - Mmap the ruleset file. Some pros and cons I can think of: PROS: - Flatbuffer files/structure has the same format on-disk and in-memory making it suitable for mmap-ing. No serialization is needed. Also it is suitable for random access. (The whole ruleset data doesn't need to paged in from disk to access a particular field). - mmaping is generally consider faster since all file IO is performed by the OS itself CONS - On a page fault, the OS will suspend the active thread (will be used on IO thread). Can this cause jank? - When an extension is loaded, we verify the ruleset. This causes a pass over the whole ruleset file once. Does this eliminate any advantages since we are reading the whole file at least once? Alternative 2: Maintain the whole ruleset data in the browser process.
,
Jan 18 2018
After some further investigation, it seems that using a mmap-ed file on UI/IO thread is discouraged since this can cause the thread to get de-scheduled. I see two other options: 1) Having the ruleset data for each extension in memory in the browser process: This seems to be the simplest and the most efficient option. As an example, a serialized flatbuffer ruleset file containing all the relevant Easylist + Easyprivacy rules is around 4 MB. The only con is the increased memory usage along with copying the data from the indexed ruleset file during the loading of the extension. (Even if we mmap the file, the whole file will have to be read for verification purposes). 2) Having the ruleset data mmap-ed in a worker thread. This would lead ruleset evaluation to be asynchronous, which is probably not what we want. I don't think I completely understand the trade-offs this brings with respect to the browser process memory usage. Generally the way mmap-ing works is that frequently used pages would be in memory, else a page fault will occur. For background here's what subresource filtering does: - In the renderer process, the ruleset mmap-ed file is accessed directly on the renderer main thread. - In the browser process, the ruleset file is memory mapped on a blocking pool and accessed asynchronously from the UI thread. But mmap-ing probably doesn't make sense for DNR, since there is no need to share the ruleset b/w different processes. I am inclined to change the current behavior from having the ruleset mmmap-ed on the IO thread to have it directly in memory (copying the ruleset data) in the browser process. Also adding csharrison@ in case he has any insights.
,
Jan 18 2018
Your analysis looks good to me, and I think I agree with you. The peak memory usage is a bit scary but if it's a problem we should see it in browser OOMs.
,
Jan 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9e6a930bcbe67d4aab2febdbbcd25babc5fa7f9 commit b9e6a930bcbe67d4aab2febdbbcd25babc5fa7f9 Author: Karan Bhatia <karandeepb@chromium.org> Date: Sat Jan 20 01:01:35 2018 DNR: Load ruleset directly in memory instead of mmap-ing it. This CL changes the ruleset file data to no longer be memory mapped. This was not ideal since mmap-ing a file on the IO thread can cause it to get de- scheduled in case of a page fault, which we do not want. This CL changes the ruleset file data to be copied and available directly in memory. BUG= 774271 , 696822 Change-Id: I1c8aae2c1d48a7bf68a2c878a227d15344588039 Reviewed-on: https://chromium-review.googlesource.com/874595 Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Reviewed-by: Ilya Sherman <isherman@chromium.org> Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#530703} [modify] https://crrev.com/b9e6a930bcbe67d4aab2febdbbcd25babc5fa7f9/extensions/browser/api/declarative_net_request/ruleset_matcher.cc [modify] https://crrev.com/b9e6a930bcbe67d4aab2febdbbcd25babc5fa7f9/extensions/browser/api/declarative_net_request/ruleset_matcher.h [modify] https://crrev.com/b9e6a930bcbe67d4aab2febdbbcd25babc5fa7f9/tools/metrics/histograms/enums.xml
,
Jan 20 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by karandeepb@chromium.org
, Oct 12 2017