987728 - Avoid main-thread IO for something that still uses our RDF code

Status: RESOLVED WORKSFORME

(Core Graveyard :: RDF, defect)

Description

[Roberto Agostino Vitillo (:rvitillo)]

It seems that {profile}\localstore.rdf is a top main-thread IO offender in terms of number of Telemetry submissions where it appears in after filtering out IO operations during startup and shutdown.

Comment 1

[Vladan Djeric (:vladan)]

See also bug 818725

Comment 2

[Roberto Agostino Vitillo (:rvitillo)]

Gavin, we might want to replace the rdf file format with json before asynchronizing the code. Do you have a strong opinion against it? If not, what would be your requirements?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Sounds great, but there are compatibility issues that make it tricky. I don't recall the specific details. Neil investigated this (and had a patch) in bug 559505, he might remember more.

Comment 4

[Axel Hecht [:Pike]]

localstore.rdf is an open dumping ground for all consumers of gecko behind an nsIRDFDataSource. There are probably a metric ton or two of addons out there that put data into localstore.rdf that isn't even remotely related to what our own tree is putting in there.

You can probably easily dismiss the .rdf part (and use ntriples, or json), but the culprit is that once you break the nsIRDFDataSource contract, all hell breaks loose in terms of add-on-compat.

Comment 5

[(no longer active)]

From the BHR data:

nsRDFXMLSerializer::SerializeProperty(nsIOutputStream *,nsIRDFResource *,nsIRDFResource *,bool,int *) (in xul.pdb)
nsRDFXMLSerializer::SerializeDescription(nsIOutputStream *,nsIRDFResource *) (in xul.pdb)
nsRDFXMLSerializer::Serialize(nsIOutputStream *) (in xul.pdb)
RDFXMLDataSourceImpl::Serialize(nsIOutputStream *) (in xul.pdb)
RDFXMLDataSourceImpl::rdfXMLFlush(nsIURI *) (in xul.pdb)
RDFXMLDataSourceImpl::Flush() (in xul.pdb)
NS_InvokeByIndex (in xul.pdb)
XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *) (in xul.pdb)
js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) (in xul.pdb)
InternalCall (in xul.pdb)
Interpret (in xul.pdb)
js::RunScript(JSContext *,js::RunState &) (in xul.pdb)
js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) (in xul.pdb)
InternalCall (in xul.pdb)
JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) (in xul.pdb)
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) (in xul.pdb)
nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) (in xul.pdb)
PrepareAndDispatch (in xul.pdb)
SharedStub (in xul.pdb)
nsTimerImpl::Fire(int) (in xul.pdb)
nsTimerEvent::Run() (in xul.pdb)
nsThread::ProcessNextEvent(bool,bool *) (in xul.pdb)
NS_ProcessNextEvent(nsIThread *,bool) (in xul.pdb)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) (in xul.pdb)
MessageLoop::RunHandler() (in xul.pdb)

The new BHR dashboard claims this is 5% of the 128ms hangs we receive on the main process in Nightly.

Comment 6

[(no longer active)]

The data is on https://s3-us-west-2.amazonaws.com/bhr-data/v1/20170510/all.html but the page is large, and it successfully crashes both Firefox and Chrome upon loading.  Beware!

Comment 7

[(no longer active)]

Bug 818725 may be our solution here...

Comment 8

[Marco Bonardo [:mak]]

Didn't we replace localstore.rdf with xulstore.json?
Are we sure the stack in comment 5 is referred to localstore.rdf that doesn't exist anymore? IT would be very surprising.

the only remaining rdfs around are mimeTypes.rdf, that actually exists in the profile but is no more used at all, xul templates (that I doubt we use in the product) an install/update.rdf. See bug 833098.
The only ones that matter are install/update.rdf.

Comment 9

[(no longer active)]

I wasn't aware that localstore.rdf is gone!  I'm glad to know that.

I'm afraid I don't know more that what I put in comment 5.  The mention of "localstore.rdf" was stemming from my lack of knowledge about it being gone (and us never deleting deprecated files from profiles ever!).  I'm afraid I don't have the cycles to look into this in more details right now, but the BHR data is available for anyone to examine so there is no reason to be blocked on me here.  :-)

Comment 10

[Florian Quèze [:florian]]

We do have localStore.rdf main thread IO during startup at http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/toolkit/components/xulstore/XULStore.js#92 when the xulstore.json file doesn't exist in the profile. See also bug 1368567.

That said, the stack in comment 5 is something written off a timer, so I agree it's unlikely to be localStore.rdf. This stack would be much more actionable if it included JS function names, like profiler stacks.

Comment 11

[(no longer active)]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> That said, the stack in comment 5 is something written off a timer, so I
> agree it's unlikely to be localStore.rdf. This stack would be much more
> actionable if it included JS function names, like profiler stacks.

We do have JS stack traces for chrome code.  I suggest exploring the data!  <https://squarewave.github.io/?search=RDFXMLDataSourceImpl&thread=0>

From what I can see, this is a problem caused by the TabMixPlus extension.  :-(

Definitely not a [qf:p1] issue though, and it will go away in Firefox 57.  I'm glad I looked more into it.

Comment 12

[(no longer active)]

Not sure what to do with this bug.  Is there a reason to keep it open?

Comment 13

[Florian Quèze [:florian]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #11)

> We do have JS stack traces for chrome code.  I suggest exploring the data! 
> <https://squarewave.github.io/?search=RDFXMLDataSourceImpl&thread=0>

I don't understand this UI well, I may ask questions about it next week.

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #12)
> Not sure what to do with this bug.  Is there a reason to keep it open?

No, let's close it.