ensure /tcktestdir created for FileConnection tests #1862
Conversation
|
The bad news is that the FileConnection tests are still failing on this branch. The good news is that they're failing consistently, at least on Travis (they pass locally). So it should be a bit easier to figure out why. |
|
I figured out how to get them to fail locally (blow away the database before running the tests), and this is actually a regression from #1201, which made the VM start running before the filesystem is initialized. When loading a midlet, that's ok, because the VM itself (and apparently the MIDletSuiteLoader) don't need the fs, and #1201 ensures that the fs will be inited in MIDletSuiteUtils.vmBeginStartUp before MIDletSuiteLoader starts the midlet. But the FileConnection tests are run by com.ibm.tck.client.TestRunner, which is a main program (specified by the main config parameter, not the midletClassName parameter). And the VM doesn't wait for the fs to be inited before starting it. Which means it can race fs initialization (causing test failures). The simplest (and most robust) fix is to revert the fs part of #1201 and make VM startup wait on fs initialization. Presumably that would regress startup perf, but I don't see any numbers in #1201, and when I run the startup benchmark for WA on desktop, it shows that doing so actually improves perf:
Ditto on a Flame (albeit insignificantly):
So I think the simple, robust fix is the best one here. |
This probably depends on the order in which we perform initialization steps.
This is definitely weird, how can this change improve startup perf? It could leave it unchanged (if the other promises in loadingPromises are resolved after the FS is initialized), but I don't see how it could improve perf! So, I'd keep the optimization from #1201 and make TestRunner a MIDlet. |
The number of files could affect fs initialization, but I'm not sure it's significant, given that we use IDBObjectStore.getAll to get their metadata, and it is presumably designed to be efficient at retrieving all records, such that the majority of its cost is unrelated to the number of records it retrieves (which is why it's so much faster than retrieving all records via a standard request).
One possibility is that there's a hidden cost to context switching between fs initialization and other work we do during VM startup (like Ion compilation). It's also possible that my results on desktop were faulty.
That would solve the problem for TestRunner, but other Java programs would still be susceptible to the underlying bug. I'd prefer a more robust solution. |
Right, I always forget that we're only loading the metadata.
I'll run the startup benchmark again on my machine as well.
OK. We could wait for the FS to be initialized somewhere else (at a point in common between normal Java programs and MIDlets), but let's do that only if the benchmark shows there's a regression. |
|
@mykmelez why did you close this PR? |
Hmm, I didn't intend to. I was doing some branch cleanup and accidentally deleted the branch for which I was requesting the pull, hence these messages about my deleting and restoring the branch:
I guess @github autotragically closed the PR when I deleted the branch and didn't reopen it when I restored it (and didn't tell me what it was doing/not doing in either case). |
|
This is my result. Baseline is without the optimization (with your patch).
|
|
I reran the tests on desktop and saw no difference this time (baseline is master, benchmark is this branch):
But I'm testing startup to the Terms screen, and my exported FS is only 4.9MB, so it's possible that a difference would appear with a larger FS or on startup to a different screen of the midlet. Nevertheless, I still don't like the optimization, since it can break midlets and other Java programs in ways that are hard to diagnose. I'd prefer correctness in this case, even if it means taking a perf hit. But I'm open to alternatives, like initializing the FS at some common point. @marco-c Any idea what that common point might be? |
|
Given that the regression is small and is only present in some cases, I think we can merge this without waiting for the alternative optimization (I've filed #1871 to investigate it). |
…ailure ensure /tcktestdir created for FileConnection tests
I think the TCK FileConnection tests fail sometimes because there's a race between two fs.init promise handlers: first, in fs-init.js, which mkdirs /tcktestdir after fs.init; and second, in fstests.js, which calls fs.clear after fs.init.
When fstests.js clears the database before fs-init.js creates /tcktestdir, then that directory exists, and the FileConnection tests pass. But when fstests.js clears the database after fs-init.js creates /tcktestdir, then that directory gets deleted after being created, so it doesn't exist when the FileConnection tests are run, and they fail.
This branch explicitly creates /tcktestdir if we're running the FileConnection tests. It also explicitly deletes the database before running those tests, so it should lead to more consistent success (or failure!) of that test suite.