[Zeitgeist-bugs] [Bug 48888] Review request: zeitgeist event caching

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=48888

Seif Lotfy <seif at lotfy.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #5 from Seif Lotfy <seif at lotfy.com> 2012-08-20 21:58:50 UTC ---
(In reply to comment #4)
> Comment on attachment 60268 [details] [review]
> event cache patch
> 
> Review of attachment 60268 [details] [review]:
> -----------------------------------------------------------------
> 
> Here's some more comments... Also, can you please include the links to the
> benchmarks you had, for future reference?
> 
> IIRC I asked for some additional benchmark (for when stuff isn't in the cache,
> or something like that), did you get around to trying that?
> 
> Thanks!
> 

IIRC I think we had benchmarked the python implementation making it faster if
the maximum elements in cache is not queried and else we skip the cache. So
pretty sure it was faster for queries with elements results < maximum cache
size...

> ::: src/db-reader.vala
> @@ +180,5 @@
> >          {
> > +            Event e = events.lookup (id);
> > +            if (e != null)
> > +                cache.cache_event (e);
> > +            results.set(i++, e);
> 
> When retrieving events from cache you've put them directly into "results" (not
> "events"), so aren't you overwritting them with null here?

Good question.

> 
> ::: src/event-cache.vala
> @@ +40,5 @@
> > +    {
> > +        Event? e = cache_table.get (id);
> > +        if (e != null)
> > +        {
> > +            lru_queue.remove_all (id);
> 
> Wouldn't remove() be faster? (There shouldn't be repeated IDs in the queue)

Good point...
Trever can you have one last look so i can merge this soon?
Cheers
Seif

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.