Created attachment 227824 [details] [review]
os-database: Remove redundant error declaration

Also remove the redundant catching of it.

Created attachment 227825 [details] [review]
os-database: Derive from GLib.Object

So we can have signals and stuff..

Created attachment 227826 [details] [review]
os-database: Load database asynchronously

Load database in a separate thread so the rest of the Application
(especially the UI) can load faster.

In my limited testing, I saw about 1 second difference in load time.

Created attachment 228058 [details] [review]
os-database: Load database asynchronously

Load database in a separate thread so the rest of the Application
(especially the UI) can load faster.

In my limited testing, I saw about 1 second difference in load time.

Review of attachment 228058 [details] [review]:

Some hard numbers on the time it takes to parse the database would make these changes much more compelling.

::: src/os-database.vala
@@ +37,3 @@
         var loader = new Loader ();
         try {
+            yield run_in_thread (() => { loader.process_default_path (); });

Imo libosinfo should get async loading functions rather than us working around it if it's so much of an issue.

@@ +43,3 @@
         try {
             loader.process_path (get_logos_db ()); // Load our custom database
+            yield run_in_thread (() => { get_logos_db (); });

The loader.process_path(...) call was lost here. Same comment about libosinfo needing a process_path_async.

Review of attachment 228058 [details] [review]:

About hard numbers, those should only be needed if the difference is not visible. As I showed to you and Marc-Andre, there is a visible (though i might be wrong about it being 1 sec) diff here.

::: src/os-database.vala
@@ +37,3 @@
         var loader = new Loader ();
         try {
+            yield run_in_thread (() => { loader.process_default_path (); });

My first attempt would be to somehow optimize loading of DB (perhaps some kind of caching). That might not be so easy so for now I'm proposing this work-around.

@@ +43,3 @@
         try {
             loader.process_path (get_logos_db ()); // Load our custom database
+            yield run_in_thread (() => { get_logos_db (); });

ouch! this was supposed to be: yield run_in_thread (() => { loader.process_path (get_logos_db ()); });

(In reply to comment #6)
> Review of attachment 228058 [details] [review]:
> 
> About hard numbers, those should only be needed if the difference is not
> visible. As I showed to you and Marc-Andre, there is a visible (though i might
> be wrong about it being 1 sec) diff here.

Sorry but that's not very visible, there might be an improvement, but that's very handwavy, and not really measurable, ...

(In reply to comment #7)
> (In reply to comment #6)
> > Review of attachment 228058 [details] [review] [details]:
> > 
> > About hard numbers, those should only be needed if the difference is not
> > visible. As I showed to you and Marc-Andre, there is a visible (though i might
> > be wrong about it being 1 sec) diff here.
> 
> Sorry but that's not very visible, there might be an improvement, but that's
> very handwavy, and not really measurable, ...

OTOH, it doesn't cause any harm to load DB async in a separate thread. I don't think this is worth spending a lot of time getting numbers and all. I know for a fact that db loading is very slow (just use any basiclibosinfo app and see how slow it is) so we do this for now and I try to look for a way to make DB loading faster. Of course, I'll fix the issue you pointed out in the patch.

After commenting out some code in osinfo-detect and running it with time(1), loading of the osinfo database takes about 300ms with a hot cache on my machine (side note, this seems to have gotten 25% slower recently), so the 1s startup improvement mentioned in the log seems quite off the mark, probably better to rework that part of the commit log.
ACK on the last patch with the missing run_in_thread added.

(In reply to comment #9)
> After commenting out some code in osinfo-detect and running it with time(1),
> loading of the osinfo database takes about 300ms with a hot cache on my machine
> (side note, this seems to have gotten 25% slower recently),

We'll need to figure out whats taking the most time. /me adds to todo

> so the 1s startup
> improvement mentioned in the log seems quite off the mark, 

Or its different on different machines?

>probably better to
> rework that part of the commit log.

Yeah, I can reword it.

Created attachment 229379 [details] [review]
os-database: Load database asynchronously

Load database in a separate thread so the rest of the Application
(especially the UI) can load faster.

In my limited testing, I saw a visible difference in load time.
Christophe did a more systematic analysis and found out it takes about
300ms with a hot cache on his machine to load the DB.

(In reply to comment #10)
> > so the 1s startup
> > improvement mentioned in the log seems quite off the mark, 
> 
> Or its different on different machines?

Our machines are similarly spec'ed, on an otherwise idle machine I don't expect this test to take three times as much time as on mine. If this was a 20% or 30% difference, then this could indeed be caused by different machines.

Review of attachment 229379 [details] [review]:

idgnc

(In reply to comment #12)
> (In reply to comment #10)
> > > so the 1s startup
> > > improvement mentioned in the log seems quite off the mark, 
> > 
> > Or its different on different machines?
> 
> Our machines are similarly spec'ed, 

There is the difference of you having twice as much RAM. :) That could make a difference, epecially since my RAM is usually mostly used even when no VM is running.

>on an otherwise idle machine I don't expect
> this test to take three times as much time as on mine. If this was a 20% or 30%
> difference, then this could indeed be caused by different machines.

Yeah, I'm not exactly claiming it was actually 1s that my patch improves.