looks good to me. Anybody else want to comment on this?

Comment on attachment 60268 [details] [review]
event cache patch

Review of attachment 60268 [details] [review]:
-----------------------------------------------------------------

::: test/direct/event-cache-test.vala
@@ +15,5 @@
> + *
> + * You should have received a copy of the GNU Lesser General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */

Nice copyrights :D

@@ +31,5 @@
> +    Test.add_func("/EventCache/expiration", expiration_test);
> +
> +    return Test.run ();
> +}
> +

You better test

[This was first reviewed at https://code.launchpad.net/~tdfischer/zeitgeist/event-caching/+merge/97450]

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!

::: 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?

::: 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)

(In reply to comment #4)
> Comment on attachment 60268 [details] [review] [review]
> event cache patch
> 
> Review of attachment 60268 [details] [review] [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

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/zeitgeist/zeitgeist/issues/1.