Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2788813002:
Try to flush DOMStorage too when ending the browsing session.
DescriptionTry to flush DOMStorage too when ending the browsing session.
Call Flush() and add DOMStorage task runners to the RundownTaskCounter in EndSession(). Provided the time taken does not exceed the timeout imposed by EndSession, waiting on these task runners should ensure that DOMStorage data is written to the filesystem.
BUG=418627, 485304
Patch Set 1 #Patch Set 2 : RundownSerially #Patch Set 3 : compile (doh!) #
Total comments: 1
Patch Set 4 : last #Patch Set 5 : compile #Patch Set 6 : compile some more #Patch Set 7 : fixes #
Total comments: 1
Messages
Total messages: 51 (33 generated)
Description was changed from ========== Add DOMStorage task runners the RundownTaskCounter used to wait for EndSession() to complete. BUG=418627 ========== to ========== Add DOMStorage task runners the RundownTaskCounter used to wait for EndSession() to complete. BUG=418627,485304 ==========
michaeln@chromium.org changed reviewers: + hashimoto@chromium.org
I'd like to revisit what we were talking about a long time ago regarding this bug, here's the old cl with some comment history. https://codereview.chromium.org/1140013003/
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Add DOMStorage task runners the RundownTaskCounter used to wait for EndSession() to complete. BUG=418627,485304 ========== to ========== Try to flush DOMStorage too when ending the browsing session. Call Flush() and add DOMStorage task runners to the RundownTaskCounter in EndSession(). Provided the time taken does not exceed the timeout imposed by EndSession, waiting on these task runners should ensure that DOMStorage data is written to the filesystem. BUG=418627,485304 ==========
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hashimoto@chromium.org changed reviewers: + gab@chromium.org
+gab@chromium.org who might be interested in disk IO during shutdown. I don't think this is what we want to have in EndSession(). EndSession() is called when OS shutdown is happening, and the OS gives chrome only a number of seconds to save important data (i.e. Preferences and Local State). Why do we need to treat DOMStorage specially while discarding all other data (e.g. history, autocomplete, downloads, bookmarks)? Preferences and Local Data are both JSON files managed by base::ImportantFileWriter whose write delay is 10 seconds. So even when we fail to save the data, we only lose changes which happened during the last 10 seconds. https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?type... If you think the data manged by DOMStorage is as important as Preferences and Local Storage, first you should write the data to the disk as frequently as base::ImportantFileWriter. IIUC the write delay of DOMStorageArea is at least 1 minute, and there is no upper limit (e.g. 10MB data can result in 1 hour delay). https://cs.chromium.org/chromium/src/content/browser/dom_storage/dom_storage_... https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... chrome/browser/browser_process_impl.cc:526: profile, base::Bind(&FlushStoragePartition, rundown_counter)); This shouldn't be put before Local State. Local State is more important than this.
On 2017/04/04 09:23:25, hashimoto wrote: > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=+gab@chromium.org who might be interested in disk IO during shutdown. > > I don't think this is what we want to have in EndSession(). > EndSession() is called when OS shutdown is happening, and the OS gives chrome > only a number of seconds to save important data (i.e. Preferences and Local > State). > Why do we need to treat DOMStorage specially while discarding all other data > (e.g. history, autocomplete, downloads, bookmarks)? > > Preferences and Local Data are both JSON files managed by > base::ImportantFileWriter whose write delay is 10 seconds. > So even when we fail to save the data, we only lose changes which happened > during the last 10 seconds. That and Local State needs to flush the bit that tells Chrome it shutdown cleanly (otherwise you get a postmortem/restore UI). > https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?type... > > If you think the data manged by DOMStorage is as important as Preferences and > Local Storage, first you should write the data to the disk as frequently as > base::ImportantFileWriter. > > IIUC the write delay of DOMStorageArea is at least 1 minute, and there is no > upper limit (e.g. 10MB data can result in 1 hour delay). > https://cs.chromium.org/chromium/src/content/browser/dom_storage/dom_storage_... > > https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... > chrome/browser/browser_process_impl.cc:526: profile, > base::Bind(&FlushStoragePartition, rundown_counter)); > This shouldn't be put before Local State. > Local State is more important than this.
> I don't think this is what we want to have in EndSession(). > EndSession() is called when OS shutdown is happening, and the OS gives chrome > only a number of seconds to save important data (i.e. Preferences and Local > State). > Why do we need to treat DOMStorage specially while discarding all other data > (e.g. history, autocomplete, downloads, bookmarks)? It's similar to preferences and local state except for web applications instead of the browser itself. > Preferences and Local Data are both JSON files managed by > base::ImportantFileWriter whose write delay is 10 seconds. > So even when we fail to save the data, we only lose changes which happened > during the last 10 seconds. > https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?type... > > If you think the data manged by DOMStorage is as important as Preferences and > Local Storage, first you should write the data to the disk as frequently as > base::ImportantFileWriter. While there is only one browser, there are an unbounded number of web applications. We (us browser developers) have explicit control over the r/w access patterns exhibited by the browser, we have no control over the access patterns exhibited by web applications. Flushing every 10 seconds has very negative consequences depending on the application so that isn't an option, see the long standing bug about excessive file IO. I'm sure more can be done to flush sooner so there's less pending at shutdown time, but there's always the possibility of having data to flush at the shutdown time too. I'd like to try to save that data. > IIUC the write delay of DOMStorageArea is at least 1 minute, and there is no > upper limit (e.g. 10MB data can result in 1 hour delay). > https://cs.chromium.org/chromium/src/content/browser/dom_storage/dom_storage_... It's rate limited based on frequency of writes and data rates, the delay can be as low as 5 secs. > https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... > chrome/browser/browser_process_impl.cc:526: profile, > base::Bind(&FlushStoragePartition, rundown_counter)); > This shouldn't be put before Local State. > Local State is more important than this. I buy that, we could arrange to start FlushStoragePartition after the browser's localstate has been written.
On 2017/04/04 22:12:22, michaeln wrote: > > I don't think this is what we want to have in EndSession(). > > EndSession() is called when OS shutdown is happening, and the OS gives chrome > > only a number of seconds to save important data (i.e. Preferences and Local > > State). > > Why do we need to treat DOMStorage specially while discarding all other data > > (e.g. history, autocomplete, downloads, bookmarks)? > > It's similar to preferences and local state except for web applications instead > of the browser itself. Preferences and Local State are important because it's needed to run the browser. OTOH DOMStorage is only needed by apps and I don't see any reason to give it higher priority than other data (e.g. history, autocomplete, downloads, bookmarks). > > > Preferences and Local Data are both JSON files managed by > > base::ImportantFileWriter whose write delay is 10 seconds. > > So even when we fail to save the data, we only lose changes which happened > > during the last 10 seconds. > > > https://cs.chromium.org/chromium/src/base/files/important_file_writer.cc?type... > > > > If you think the data manged by DOMStorage is as important as Preferences and > > Local Storage, first you should write the data to the disk as frequently as > > base::ImportantFileWriter. > > While there is only one browser, there are an unbounded number of web > applications. We (us browser developers) have explicit control over the r/w > access patterns exhibited by the browser, we have no control over the access > patterns exhibited by web applications. Flushing every 10 seconds has very If the IO is caused by a misbehaving app, it's the app's fault. Probably, we should just disable an app when we detect it is behaving badly? > negative consequences depending on the application so that isn't an option, see > the long standing bug about excessive file IO. In crbug.com/176727, I see a number of users complaining about the said problem, but I'm not sure how serious it is (the bug is marked as P2). Do you have any data which tells us that the amount of data written by DOMStorage exceeds a certain limit? BTW, do you know how other browsers are handling this problem? > > I'm sure more can be done to flush sooner so there's less pending at shutdown > time, but there's always the possibility of having data to flush at the shutdown > time too. I'd like to try to save that data. Generally you shouldn't rely on clean shutdown to save data which you think is important (e.g. crbug.com/639244 is about removing graceful shutdown from renderers). If you think the data should be persisted, you should write it to the disk more frequently. This also makes it robust against events like Chrome crash, OS crash, and device power loss. > > > IIUC the write delay of DOMStorageArea is at least 1 minute, and there is no > > upper limit (e.g. 10MB data can result in 1 hour delay). > > > https://cs.chromium.org/chromium/src/content/browser/dom_storage/dom_storage_... > > It's rate limited based on frequency of writes and data rates, the delay can be > as low as 5 secs. What matters here is the upper limit of write delay which DOMStorageArea doesn't have. The current implementation allows arbitrarily large amount of data to be buffered, and this buffered data will be lost when Chrome crashes, OS crashes, the device loses power, or EndSession exceeds the timeout set by the OS (w/ the proposed change). > > > > https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... > > File chrome/browser/browser_process_impl.cc (right): > > > > > https://codereview.chromium.org/2788813002/diff/40001/chrome/browser/browser_... > > chrome/browser/browser_process_impl.cc:526: profile, > > base::Bind(&FlushStoragePartition, rundown_counter)); > > This shouldn't be put before Local State. > > Local State is more important than this. > > I buy that, we could arrange to start FlushStoragePartition after the browser's > localstate has been written.
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> In crbug.com/176727, I see a number of users complaining about the said problem, > but I'm not sure how serious it is (the bug is marked as P2). > Do you have any data which tells us that the amount of data written by > DOMStorage exceeds a certain limit? How much DOMStorage is used is application dependent. My officemate characterized the file io of a browser left running mostly idle. Prior to the rate limiting changes, DOMStorage was the 3rd largest consumer (https://bugs.chromium.org/p/chromium/issues/detail?id=176727#c69). The rate limiting helped a lot. > BTW, do you know how other browsers are handling this problem? Looks like moz tries to persist data. See https://github.com/mozilla/gecko-dev/blob/master/widget/windows/nsWindow.cpp > What matters here is the upper limit of write delay which DOMStorageArea doesn't > have. > The current implementation allows arbitrarily large amount of data to be > buffered, and this buffered data will be lost when Chrome crashes, OS crashes, > the device loses power, or EndSession exceeds the timeout set by the OS (w/ the > proposed change). With the size and rate limits, the max delay is about an hour. Of course we can't flush caches in the face of OS or program crashes but for expected cases, we should try. > > I buy that, we could arrange to start FlushStoragePartition after the > > browser's localstate has been written. I've rearranged the order such that DOMStorage flushing doesn't start until after the other data has been stored.
On 2017/04/11 02:01:07, michaeln wrote: > > In crbug.com/176727, I see a number of users complaining about the said > problem, > > but I'm not sure how serious it is (the bug is marked as P2). > > Do you have any data which tells us that the amount of data written by > > DOMStorage exceeds a certain limit? > > How much DOMStorage is used is application dependent. My officemate > characterized the file io of a browser left running mostly idle. Prior to the > rate limiting changes, DOMStorage was the 3rd largest consumer > (https://bugs.chromium.org/p/chromium/issues/detail?id=176727#c69). The rate > limiting helped a lot. One browser developer does not represent the entire user base. > > > BTW, do you know how other browsers are handling this problem? > > Looks like moz tries to persist data. See > https://github.com/mozilla/gecko-dev/blob/master/widget/windows/nsWindow.cpp Sorry, I'm not familiar with mozilla's code base. I see it's doing something when handling WM_ENDSESSION, but not sure what it's doing. > > > What matters here is the upper limit of write delay which DOMStorageArea > doesn't > > have. > > The current implementation allows arbitrarily large amount of data to be > > buffered, and this buffered data will be lost when Chrome crashes, OS crashes, > > the device loses power, or EndSession exceeds the timeout set by the OS (w/ > the > > proposed change). > > With the size and rate limits, the max delay is about an hour. Of course we > can't flush caches in the face of OS or program crashes but for expected cases, > we should try. As I said, other browser components are doing fine without doing anything. If the IO is caused by unimportant random web apps, isn't it the apps' fault? BTW, do you have any data about which app is responsible for this bad behavior? > > > > I buy that, we could arrange to start FlushStoragePartition after the > > > browser's localstate has been written. > > I've rearranged the order such that DOMStorage flushing doesn't start until > after the other data has been stored.
mek@chromium.org changed reviewers: + mek@chromium.org
https://codereview.chromium.org/2788813002/diff/120001/content/browser/dom_st... File content/browser/dom_storage/dom_storage_context_wrapper.cc (right): https://codereview.chromium.org/2788813002/diff/120001/content/browser/dom_st... content/browser/dom_storage/dom_storage_context_wrapper.cc:257: // TODO(michaeln): Add mojo impl task runners to flush_runners I'm a bit confused about the implied semantics of the list of task runners being returned. Is it assumed that all async tasks that are necesary to flush data to disk have been queued when Flush returns? Because that's going to be hard to guarantee with the mojo implementation, with its multiple layers of mojo services on top of each other... But anyway, for now the mojo impl would (potentially) involve at least tasks run on the FILE and DB threads. But most of that is hidden behind mojo interfaces.
I've attached a manual test page to crbug 485304, here's a link to it. https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=279899 onload, the state is set to running onbeforeclose, the state is set to shutdown To reproduce the problem... * sign in to CrOS with no tabs restored, a clean boot * load that page into CrOS as the only tab open * wait 15 seconds to ensure the localStorage value written during onload is flushed * sign out of CrOS without closing the tab * sign back in and revisit this page * BUG - if it says "state was running" instead of "state was shutdown", thats the bug It's clearly a bug. Ideally i think the browser should give some time for tabs to close prior to proceeding to flush data. If all tabs don't close within [n] seconds, proceed to flushing data, and if that doesn't complete within [n] seconds, then exit.
> Is it assumed that all async tasks that are necesary to flush data to > disk have been queued when Flush returns? Almost, it's assumed that the first task in the series of tasks to flush has been posted to the first task runner in the list. And the first task will post tasks to the second task runner in the list (and so). So if Flushes caller posts tasks along the same route, the flush we be complete by the time the last task along the route executes. You have to follow them serially, it's not enough to execute tasks in parallel.
@marijn, another issue with the mojo impl and the synchronous EndSession shutdown sequence here is that values written in unload handlers by tabs that do manage to close will never be received by the main browser. The LevelDBWrapperImpl objects run on the main thread, but the EndSession function is blocking the main thread, so we won't receive any Put() methods prior to the process being exited. If we bound those objects to the IO thread instead, that would be more similar to the current impl.
On 2017/04/18 at 01:47:31, michaeln wrote: > @marijn, another issue with the mojo impl and the synchronous EndSession shutdown sequence here is that values written in unload handlers by tabs that do manage to close will never be received by the main browser. The LevelDBWrapperImpl objects run on the main thread, but the EndSession function is blocking the main thread, so we won't receive any Put() methods prior to the process being exited. If we bound those objects to the IO thread instead, that would be more similar to the current impl. Hmm, I see... moving the whole mojo localstorage implementation to live on some other thread should be straight forward enough. And hopefully it would then be enough to wait on whatever thread we move this to + the DB thread (where the actual mojo leveldb service currently lives). That DB thread might involve running tasks on the FILE thread as well, but I think all of that is done synchronously/blocking, so the DB thread should be enough to wait on. I guess one open question then is if just having called LevelDBDdatabase::Write is enough to ensure data actually got stored, or if that write needs to be a "sync" write. Unrelated question, from the storage side it seems an easier API that returning an ordered list of task runners would be to pass a callback to Flush() that gets called when flushing is complete, but I imagine that would be harder to integrate in the shutdown blocking code?
Drive-by as you're discussing places to run tasks. Please consider a TaskScheduler-provided TaskRunner (base/task_scheduler/post_task.h). We're currently migrating old APIs to it and it'd be great if new ones went straight there instead (let us -- scheduler-dev@ -- know if you hit hurdles doing so).
On 2017/04/18 at 19:10:47, gab wrote: > Drive-by as you're discussing places to run tasks. Please consider a TaskScheduler-provided TaskRunner (base/task_scheduler/post_task.h). We're currently migrating old APIs to it and it'd be great if new ones went straight there instead (let us -- scheduler-dev@ -- know if you hit hurdles doing so). TaskScheduler provided TaskRunner definitely makes things easier in general, yes. Not really related to this CL (other than that it's about the DB and FILE thread usage of the mojo localstorage implementation), it's not entirely clear from the API how I can get two single threaded task runners that are guaranteed to not run on the same thread. Maybe creating one of them with WithBaseSyncPrimitives() would be enough to ensure that tasks from other task runners don't get posted to that thread? Basically it's not clear from the task scheduler docs how you can ensure that certain tasks don't run on thread certain other single threaded task runners run on. (there's actually a third TaskRunner involved on the leveldb side, that also needs to be able to post task to the one single threaded task runner that might block).
On 2017/04/18 19:32:18, Marijn Kruisselbrink wrote: > On 2017/04/18 at 19:10:47, gab wrote: > > Drive-by as you're discussing places to run tasks. Please consider a > TaskScheduler-provided TaskRunner (base/task_scheduler/post_task.h). We're > currently migrating old APIs to it and it'd be great if new ones went straight > there instead (let us -- scheduler-dev@ -- know if you hit hurdles doing so). > > TaskScheduler provided TaskRunner definitely makes things easier in general, > yes. Not really related to this CL (other than that it's about the DB and FILE > thread usage of the mojo localstorage implementation), it's not entirely clear > from the API how I can get two single threaded task runners that are guaranteed > to not run on the same thread. Maybe creating one of them with > WithBaseSyncPrimitives() would be enough to ensure that tasks from other task > runners don't get posted to that thread? Basically it's not clear from the task > scheduler docs how you can ensure that certain tasks don't run on thread certain > other single threaded task runners run on. (there's actually a third TaskRunner > involved on the leveldb side, that also needs to be able to post task to the one > single threaded task runner that might block). Questions: 1) Do you need single-threads (SingleThreadTaskRunner) or mere ordering + exclusion for thread-safety is fine (SequencedTaskRunner)? 2) Is exclusion why you need two "threads"? If so two SequencedTaskRunner will give you those properties, if you *really* need two physical threads than calling base::CreateSingleThreadTaskRunnerWithTraits() will give you two physical threads for now (we'd like to avoid that -- please let us know if that's mandatory for you so we understand and weigh in your use case). A third task runner is no problem, can come from same API. FWIW: DB and FILE thread are being deprecated in favor of TaskScheduler as well. The only reason to add things to them is if you need to be exclusive with something else that already runs on them (though we'd be even happier if you took that thing out of its named thread onto a TaskScheduler sequence :)). FWIW2: IndexedDB will move to a SequencedTaskRunner managed by TaskScheduler (http://crbug.com/552552) the minute I complete making base::Timer sequence-safe (https://codereview.chromium.org/2491613004/).
On 2017/04/18 at 19:40:00, gab wrote: > On 2017/04/18 19:32:18, Marijn Kruisselbrink wrote: > > On 2017/04/18 at 19:10:47, gab wrote: > > > Drive-by as you're discussing places to run tasks. Please consider a > > TaskScheduler-provided TaskRunner (base/task_scheduler/post_task.h). We're > > currently migrating old APIs to it and it'd be great if new ones went straight > > there instead (let us -- scheduler-dev@ -- know if you hit hurdles doing so). > > > > TaskScheduler provided TaskRunner definitely makes things easier in general, > > yes. Not really related to this CL (other than that it's about the DB and FILE > > thread usage of the mojo localstorage implementation), it's not entirely clear > > from the API how I can get two single threaded task runners that are guaranteed > > to not run on the same thread. Maybe creating one of them with > > WithBaseSyncPrimitives() would be enough to ensure that tasks from other task > > runners don't get posted to that thread? Basically it's not clear from the task > > scheduler docs how you can ensure that certain tasks don't run on thread certain > > other single threaded task runners run on. (there's actually a third TaskRunner > > involved on the leveldb side, that also needs to be able to post task to the one > > single threaded task runner that might block). > > Questions: > 1) Do you need single-threads (SingleThreadTaskRunner) or mere ordering + exclusion for thread-safety is fine (SequencedTaskRunner)? I'm not sure the exact reasoning, but yes, it seems currently all mojo interfaces require single-threads (the runner argument passed to InterfacePtr::Bind and simlar methods in other mojo classes). > 2) Is exclusion why you need two "threads"? If so two SequencedTaskRunner will give you those properties, if you *really* need two physical threads than calling base::CreateSingleThreadTaskRunnerWithTraits() will give you two physical threads for now (we'd like to avoid that -- please let us know if that's mandatory for you so we understand and weigh in your use case). Yeah, creating physical threads for every SingleThreadTaskRunner seems unfortunate (but with the current API seems to be the only way to avoid potential deadlocks, as there doesn't seem to be a way to specify which task runners actually need to be independent of each other). "Fixing" mojo to not require single threaded task runners would go a long way to getting id of the need for SingleThreadTaskRunners in my case. The reason the code then ends up using things like WaitableEvent is because leveldb itself expects to make synchronous calls to methods implemented by me (from multiple threads/task runners), where those methods need to make synchronous mojo calls (and those mojo calls have to all happen from the same thread due to mojo limitations). So currently I have three task runners: DB Thread: - implements LevelDBDatabase mojo interface, calls out to actual leveldb code, needs to be able to block on file operations (which are scheduled on the FILE runner) BG Runner: - leveldb code can schedule tasks on this runner. order doesn't matter, but tasks need to be able to block on file operations (which are scheduled on the FILE runner) FILE Runner: - calls out to mojo file service for actual file operations (where the mojo service implementing those operations actually also lives on the FILE thread, but that's not required, just nice to avoid extra thread hops) So with the current API it seems like the DB thread and BG runner would need to have WithBaseSyncPrimitives in their task traits (oops, currently missing from the tasks scheduled on the DB runner, I guess that code isn't tested very well yet on my side), and the DB and FILE runners would need to be SingleThreadTaskRunners. > A third task runner is no problem, can come from same API. > > FWIW: DB and FILE thread are being deprecated in favor of TaskScheduler as well. The only reason to add things to them is if you need to be exclusive with something else that already runs on them (though we'd be even happier if you took that thing out of its named thread onto a TaskScheduler sequence :)). It seems another reason for now is if you don't want to add the overhead of X extra threads, because the need to create mojo interfaces... But this is all getting a bit off topic for this CL, so maybe it makes more sense to continue this discussion on some mailing list?
> Yeah, creating physical threads for every SingleThreadTaskRunner seems > unfortunate (but with the current API seems to be the only way to avoid > potential deadlocks, as there doesn't seem to be a way to specify which task > runners actually need to be independent of each other). "Fixing" mojo to not > require single threaded task runners would go a long way to getting id of the > need for SingleThreadTaskRunners in my case. +1 "fixing" that aspect of mojo > The reason the code then ends up using things like WaitableEvent is because > leveldb itself expects to make synchronous calls to methods implemented by me > (from multiple threads/task runners), where those methods need to make > synchronous mojo calls (and those mojo calls have to all happen from the same > thread due to mojo limitations). So currently I have three task runners: > > DB Thread: > - implements LevelDBDatabase mojo interface, calls out to actual leveldb code, > needs to be able to block on file operations (which are scheduled on the FILE > runner) > > BG Runner: > - leveldb code can schedule tasks on this runner. order doesn't matter, but > tasks need to be able to block on file operations (which are scheduled on the > FILE runner) > > FILE Runner: > - calls out to mojo file service for actual file operations (where the mojo > service implementing those operations actually also lives on the FILE thread, > but that's not required, just nice to avoid extra thread hops) Does the DBThread block on the BGRunner too, there's a Mutex class in leveldatabase/port/port.h leveldb logic primarily runs on the DBThread leveldb compaction logic runs on the BGRunner leveldb logic on both of those threads require synchronous file access
On 2017/04/18 21:52:04, michaeln wrote: > > Yeah, creating physical threads for every SingleThreadTaskRunner seems > > unfortunate (but with the current API seems to be the only way to avoid > > potential deadlocks, as there doesn't seem to be a way to specify which task > > runners actually need to be independent of each other). "Fixing" mojo to not > > require single threaded task runners would go a long way to getting id of the > > need for SingleThreadTaskRunners in my case. > > +1 "fixing" that aspect of mojo Right, we're currently making core APIs sequence-friendly, we've gone through most of //base and //net but we're still missing mojo (http://crbug.com/678155). Once you can hold your mojo service on a sequence you'll be fine IMO. Sequences are all independent (can't deadlock -- unless the pool is somehow flooded with independent waiters but we have ways to prevent that from happening). They do support waiting but you should really avoid it at all costs (prefer async replies if you can at all help it) -- that's in Chromium overall not just TaskScheduler. > > > The reason the code then ends up using things like WaitableEvent is because > > leveldb itself expects to make synchronous calls to methods implemented by me > > (from multiple threads/task runners), where those methods need to make > > synchronous mojo calls (and those mojo calls have to all happen from the same > > thread due to mojo limitations). So currently I have three task runners: > > > > DB Thread: > > - implements LevelDBDatabase mojo interface, calls out to actual leveldb code, > > needs to be able to block on file operations (which are scheduled on the FILE > > runner) > > > > BG Runner: > > - leveldb code can schedule tasks on this runner. order doesn't matter, but > > tasks need to be able to block on file operations (which are scheduled on the > > FILE runner) > > > > FILE Runner: > > - calls out to mojo file service for actual file operations (where the mojo > > service implementing those operations actually also lives on the FILE thread, > > but that's not required, just nice to avoid extra thread hops) > > Does the DBThread block on the BGRunner too, there's a Mutex class in > leveldatabase/port/port.h > > leveldb logic primarily runs on the DBThread > leveldb compaction logic runs on the BGRunner > leveldb logic on both of those threads require synchronous file access |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
