Follow-on from Bug 1074340. The current approach is pretty byzantine.

This moves all of our caching to the Map<String,GeckoProfile> inside GeckoProfile. I even moved the guest profile caching there as well. The weirdest bit here is that we previously parsed the args in GeckoApp.onCreate and used those to set mProfile. I think we probably have some options on what we do there, but I moved the arg parsing into GeckoProfile itself. The first time get() is called, we'll try to pull a profile name/dir out of there AND cache that profile in the Map. From then on, we should fast path through GeckoProfile.get() using the Map. I made getGuestDir non-lazy. Its only called during guest profile construction. Seems silly to optimize it. If you want the dir, getGuestProfile().getDir() should be just as fast... I also made GeckoThread "always" pass the profile dir. i.e. I'd like to move away from Gecko touching profiles.ini at all. Interestingly, it also used to use getCanonicalPath(). I guess that's an upfront cost to make future accesses faster (although, I'd hope that Gecko itself normalizes and caches the normalized profile dir, probably faster than java does)? I removed it just to clean up the try catch, but we could put it back. Do either of you know if it was worth it?

Comment on attachment 8504330 [details] [diff] [review] Patch Review of attachment 8504330 [details] [diff] [review]: ----------------------------------------------------------------- A couple of concurrency points, not a full review. The main thrust of my comments: we're anticipating a lot of lookups here, and I'd like to avoid the need to synchronize on the fast path. ::: mobile/android/base/BrowserApp.java @@ +805,5 @@ > // trigger profile init too early. > ThreadUtils.postToBackgroundThread(new Runnable() { > @Override > public void run() { > + if (GeckoProfile.get(BrowserApp.this).inGuestMode()) { I think it's legitimate to cache the profile -- and, indeed, whether we're in guest mode, if you wish -- in an AtomicReference or a volatile on GeckoApp. This is especially legitimate if GeckoProfile.get continues to be heavily synchronized. ::: mobile/android/base/GeckoProfile.java @@ +41,5 @@ > public static final String DEFAULT_PROFILE = "default"; > public static final String GUEST_PROFILE = "guest"; > > + // XXX: Do we ever actually have more than one profile? Is it worth keeping a map here? > + private static ConcurrentHashMap<String, GeckoProfile> sProfileCache = new ConcurrentHashMap<String, GeckoProfile>(1); Given that we're using a CHM here, I think we can avoid some synchronization below, which will significantly help throughput in the New World of looking up the profile all the time. @@ +139,5 @@ > + } > + } > + > + public synchronized static GeckoProfile get(Context context) { > + synchronized (sDefaultProfileName) { Instead of synchronized, do something like this: GeckoProfile p = sProfileCache.get(computedName); if (p != null) { return p; } return sProfileCache.putIfAbsent(computedName, new GeckoProfile(...)); It's possible that two GeckoProfile instances will be created, but only one will ever be used. If you really want to stop two instances even being created, you could break out the *creation and insertion* part into a separate method. This is one case where double-checked locking is A-OK, because you're still able to use an atomic operation on sProfileCache. @@ +141,5 @@ > + > + public synchronized static GeckoProfile get(Context context) { > + synchronized (sDefaultProfileName) { > + // NOTE: This will make us throw quickly if a null context was passed in > + final Context appContext = context.getApplicationContext(); Do this first. @@ +155,5 @@ > + // it from the webapp activity, pull it from args passed to the context, or just fall > + // back to "default". > + if (context instanceof WebappImpl) { > + final WebappImpl webapp = (WebappImpl) context; > + sDefaultProfileName = webapp.getDefaultProfileName(); Can we extract out a getProfileName method, so that these get(Context, ...) methods are easier to read and analyze? @@ +229,5 @@ > + } > + } > + > + public synchronized static String getDefaultProfileName() { > + synchronized (sDefaultProfileName) { How about using an AtomicReference for sDPN? @@ +297,5 @@ > } > > private static File getGuestDir(Context context) { > + return context.getFileStreamPath("guest"); > +} Indenting. @@ +314,5 @@ > > public static GeckoProfile getGuestProfile(Context context) { > + GeckoProfile profile = sProfileCache.get(GUEST_PROFILE); > + > + if (profile == null) { Invert conditional.

(In reply to Richard Newman [:rnewman] from comment #2) > The main thrust of my comments: we're anticipating a lot of lookups here, > and I'd like to avoid the need to synchronize on the fast path. Sorry about that. I was waffling on how to synchronize this and left way too much in. I changed it up a bit here, but I still have a bit in. Thanks for the help. > return sProfileCache.putIfAbsent(computedName, new GeckoProfile(...)); All this gains is "locking" when we don't want to overwrite something already in the cache, right? I basically don't care about that. i.e. you can currently overwrite something in the cache at runtime. I don't really want to force GeckoProfile to be singleton's here. I'm just trying to avoid doing any file i/o that we don't need. > How about using an AtomicReference for sDPN? I don't think this provides the protection I really wanted. i.e. I was trying to ensure that if, at startup, two threads quickly called getProfile() with different context's (I'm not sure how that would happen...) only one would set a default, the second would wait and then use the new default. I've given up on that a bit here now. i.e. its possible that both will still wind up setting the default. I left a little in so that methods that get/set the defaultProfile are protected, but the setters shouldn't be hit often if things are working, and the getters do nothing so they should be fast. A bigger worry might be that a Service is running with a non-Activity context. We'd call getProfile in there, which would fall back to "default". If a real GeckoApp activity was then started in the same process, it would default to that profile name, regardless of what arguments were passed to it. The only good fix I have for that type of situation is to force-reset the defaults in onCreate for a GeckoApp. That's what I did here. It just kills any cached info in onCreate()

This changes the profile Map-cache to just a single instance cache. Since you're worried a bit about the speed of lookups, I wondered if this would be better. I can't imagine its often we have more than one profile alive at once anyway.

Answering these, if not yet reviewing the new patches: (In reply to Wesley Johnston (:wesj) from comment #3) > > return sProfileCache.putIfAbsent(computedName, new GeckoProfile(...)); > > All this gains is "locking" when we don't want to overwrite something > already in the cache, right? I basically don't care about that. i.e. you can > currently overwrite something in the cache at runtime. I don't really want > to force GeckoProfile to be singleton's here. I'm just trying to avoid doing > any file i/o that we don't need. What it buys is that only one instance of a GeckoProfile ever escapes into the wild, yes. I figure that's a good thing, because the GP is now the owner of a single LocalBrowserDB, which is the singleton owner of a SuggestedSites, a TabsAccessor, a BaseURLMetadata, a BaseSearches, and so on. (Bug 1077590.) What I'd be hoping to avoid by making GeckoProfile a lazy collection of singletons (if only optionally) is some access pattern resulting in two GPs supposedly representing, say, your TabsAccessor, or your SuggestedSites, and thus Extreme Weirdness® happening that's very hard to reproduce. > I've given up on that a bit here now. i.e. its possible > that both will still wind up setting the default. It's OK (not ideal, but OK) that two threads both create a GP. We just want them both to then walk away with the same one. Both AtomicReference and CLQ.putIfAbsent will achieve that. If they both create a GP and then both keep separate references, I think we open ourselves up to bugs. > A bigger worry might be that a Service is running with a non-Activity > context. We'd call getProfile in there, which would fall back to "default". > If a real GeckoApp activity was then started in the same process, it would > default to that profile name, regardless of what arguments were passed to it. Our services are supposed to all know which profile they're using, and typically directly access the CP with that profile name. I don't think we can count on those implicitly finding out which profile is "active", and having them futz with GP's global state seems like a bad idea.

Comment on attachment 8504920 [details] [diff] [review] Patch v2 Review of attachment 8504920 [details] [diff] [review]: ----------------------------------------------------------------- Some problems with the GeckoProfile part of this, so here's a review of that file. ::: mobile/android/base/GeckoProfile.java @@ +118,5 @@ > + } > + > + private static String parseArg(String args, String pattern) { > + // Look for a name passed in > + if (args.contains("-P")) { Is this right? This means you can't pass -profile without -P. @@ +150,5 @@ > + * Sets up the default profile based on the type of activity passed in or the incoming intent args. > + * This will overwrite any current default profile name. > + */ > + private static String setupDefaults(final Context context) { > + synchronized (sDefaultProfileName) { This isn't right. You actually change the object when you assign in parseArgs, so callers won't necessarily be synchronizing on the same object! @@ +204,1 @@ > sProfileCache.put(profileName, profile); There's a race here unless you use putIfAbsent, and you should pay attention to the return value. We don't want to get and use two different GeckoProfile instances at the same time. @@ +212,5 @@ > + } > + > + public static String getDefaultProfileName() { > + synchronized (sDefaultProfileName) { > + if (sDefaultProfileName == null) { These two lines are fundamentally at odds. You can't synchronize on a null object reference.

Comment on attachment 8504924 [details] [diff] [review] Patch 2/2 Review of attachment 8504924 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure about this. Let's revisit after addressing the first patch.

I know we landed some cleanup here, so I'm not sure if this is worth doing anymore. Pinging rnewman just so he'll remember this :)

Yeah, I think we did a lot of this.