TechEmpower / FrameworkBenchmarks

Source for the TechEmpower Framework Benchmarks project
https://www.techempower.com/benchmarks/
Other
7.62k stars 1.94k forks source link

RevenJ making bulk requests for Queries and Update tests #2204

Closed knewmanTE closed 8 years ago

knewmanTE commented 8 years ago

RevenJ is getting very high results for Queries and Update because it is making bulk requests, rather than making each request individually, as the TFB docs specify (see number 6). We're currently hiding the RevenJ results from the official listings until this is resolved.

public World[] loadWorlds(final int count) throws IOException {
        bulkReader.reset();
        for (int i = 0; i < count; i++) {
            callables[i] = bulkReader.find(World.class, Integer.toString(getRandom10k()));
        }
        bulkReader.execute();
        try {
            for (int i = 0; i < count; i++) {
                buffer[i] = ((Optional<World>) callables[i].call()).get();
            }
        } catch (Exception e) {
            throw new IOException(e);
        }
        return buffer;
    }

(Source)

zapov commented 8 years ago

Rule 6 states: "Since this test is designed to exercise multiple queries, each row must be selected individually by a query. It is not acceptable to retrieve all required rows using a SELECT ... WHERE id IN (...) clause."

Let me explain why Revenj is not violating this nor in the spirit, nor in the rules as stated above: 1) Revenj doesn't translate that query into id IN clause, but into multiple subqueries where each subquery is a separate "column" 2) If purpose of that test is to simulate loading various data from various different sources, bulk API used by Revenj will work without any modification; therefore Revenj follows the spirit of the test in doing more DB operations (while knowing how to do it at once) 3) Revenj is doing something no other framework knows how to do. This alone is not good enough reason to dismiss it's behavior. If anything others should learn from it, instead of accusations of breaking the rules 4) Revenj will translate that query into something along the lines of SELECT ROW( (subquery1), (subquery2),...) where specific subquery can be arbitrary complex (singe row/multiple rows)... The resulting response from Postgres is not something you can easily parse, so trying to do a manual implementation of that is futile

I understand that this is something you have never seen, or probably were not aware it can be done, but I don't see anything in the rules which prevents doing that. I will also point out that bulk reading is normal in various databases (MSSQL). So, if you expect implementation to be "production quality" your requirements should reflect "production usage". Doing bulk operations on DB is normal production usage and something always suggested by and DBA.

An example of benchmark with more complex domains you can find at: https://github.com/ngs-doo/dal-benchmark

Regards, Rikard

zapov commented 8 years ago

And some more info to help you "fix" your stance on what Revenj is doing.

Tutorial about bulk read feature: https://github.com/ngs-doo/revenj/blob/master/tutorials/revenj-tutorial-bulk-reading.md

Your rules state queries, but here you are complaining about requests. query != request

I really dislike that you hid the result without consulting me or the community; but it's not like I wasn't expecting that ;(

Regards, Rikard

methane commented 8 years ago

I agree that RevenJ's bulk read is unique and interesting. But benchmark fairness is more important than such uniqueness.

It may violates the rule "each row must be selected individually by a query.". Even using subquery, it fetches multi rows by a query.

So I think the rule should be more clarified.

zapov commented 8 years ago

I don't seem to follow the logic why Revenj implementation is unfair to others? Because it's more advanced and more faster? It doesn't break any rules as stated currently. subquery is a query. It doesn't do IN (ids) in an alternative way, but supports loading of different tables. This should be real purpose of this test.

The underlying use case for this test is: How long does it take to do a server request when you have to build up a complex screen consisting from 20 different queries?

The issue is that TE thought you have to do multiple requests to the database to obtain such results. And to simplify the benchmark, instead of creating 500 different tables they've used a single table and forbid the loading of multiple rows from that table.

Obfuscated way of doing IN (ids) is

SELECT t1.* FROM table t1 where t1.id = $id1 UNION ALL SELECT t2.* FROM table t2 where t2.id = $id2

Revenj is not doing that, therefore Revenj is neither cheating, nor breaking the rules. They should be hiding Hibernate results since it uses first level cache instead of asking DB for results. And LWAN who doesn't do requests to remote server. But instead only Revenj.JVM results were removed, because they were "too good". That's childish behavior.

Bottom line is that Revenj doesn't break this test, nor in the spirit of the test, nor in the currently stated rules. And again, it would be better to learn from other people instead of calling them "researcher fail..." That "researcher" also built Java JSON libraries which are often faster than most binary codecs (Kryo, Protobuf, Flatbuffers): https://github.com/eishay/jvm-serializers/wiki and frankly it's tiresome explaining to developers that their understanding of performance is "mostly driven by beliefs".

methane commented 8 years ago

I don't seem to follow the logic why Revenj implementation is unfair to others? Because it's more advanced and more faster?

No. Because the rule doesn't clarify how to treat subquery.

subquery is a query.

Yes. But query containing multiple query is also a query. Subqueries doesn't fetch row, they only query about row. And single root query fetches all rows. So it may or may not break the rule. The rule is unclear about subquery.

I'm not against about accepting Revenj's result. Just saying rule should be clear about it.

zapov commented 8 years ago

Postgres doesn't have multiple results sets feature. Other DBs do. That makes your point "only single root query fetches all rows" kind of moot. As explained in the docs, Revenj idiomatic way is to let it create a function with multiple queries (which is not allowed in this bench). MSSQL will let you pass in batch queries.

Bottom line is that if the tests represents fetching 20 different data sources, Revenj abides by this rule. If test requires 20 roundtrips to the database this goes against "production usage" of the database and therefore doesn't make any sense.

So the only way to make Revenj break that rule is by changing the rules so that the frameworks have to do multiple requests to the database, so that other frameworks not capable of doing multiple read queries in a batch are on the same playing field.

That seems kind of childish? Doesn't it?

methane commented 8 years ago

Postgres doesn't have multiple results sets feature. Other DBs do.

Ah! All of Python and Go benchmarks doesn't use multiple resultset. I'll use it when this is accepted by the rule explicitly. Thank you for pointing it out.

If test requires 20 roundtrips to the database this goes against "production usage" of the database and therefore doesn't make any sense.

Many round trips are very popular on large "production" applications. So I think "test requires 20 roundtrip" doesn't go against "production usage", and make sense.

For example, SELECT id, token, banned FROM user WHERE name=? to check user existence, then SELECT notice WHERE user_id=? to select notice messages. In such case, queries won't be batched, because first query and second query are sent from different part of application, and second part may not be executed if first part fails.

So the only way to make Revenj break that rule is by changing the rules so that the frameworks have to do multiple requests to the database, so that other frameworks not capable of doing multiple read queries in a batch are on the same playing field.

That seems kind of childish? Doesn't it?

I think changing rule to more explicit for fair benchmark is good, regardless the rule will allow or disallow query batching.

zapov commented 8 years ago

The point is you will do N queries until you have a performance problem and than you will optimize it into smaller number of queries. What goes against production usage is not allowing frameworks to show that they can do batch reading.

The bottom line is this: is allowing multiple queries in same request beneficial to users looking at the results or not? And I think it's obvious what the answer to that question is.

I fully agree with not allowing IN (ids). But pipelining is allowed, batch update is allowed, can't understand the rationale for not allowing batch reading. If anything I think batch update should not be allowed, unless it works across different tables (eg. chained CTEs). Also, it's not implied that batching is faster. Oracle for example doesn't like when you send it multiple batched read queries (depending on circumstances).

bhauer commented 8 years ago

I have clarified the requirements for the Multi-query test as discussed on the Google Group mailing list here:

https://groups.google.com/d/msg/framework-benchmarks/nePDNY9jp-4/EtG6SswmFQAJ

Note that the intent of the Multi-query test has always been to make N round-trips to an external database server. A single table is used as a simplification, as is a random number generator for selecting which row to retrieve in each iteration. But the intent of the test type is to act as a simulation of the common real-world scenario where multiple round-trips to a database (or, for that matter, any external system) are needed. Imagine a set of branching logic that necessitates a sequence of round-trips and doesn't present the luxury of being able to execute all queries as a bundle ahead of time.

It may be worthwhile to add another test type that executes multiple queries in tandem (either as a batch of statements of within a single statement). If that is interesting to others, we should add that to issue #133.

zapov commented 8 years ago

I think you fail to understand that your thinking of having to do N roundtrips to the database is a broken one. If you took a look at my link which I added you could see that Revenj doesn't have to do N roundtrips for various scenarios. This allows you to implement branching logic within same request.

And this is normal production usage of Revenj.

My underlying point is that it would be beneficial to this benchmark if it allowed multiple result set implementations since this is normal production usage. It will expose technologies and frameworks which have issues with such a feature.

methane commented 8 years ago

(Sorry, slightly off topic)

@zapov

This allows you to implement branching logic within same request.

Wow, that's great! But tutorial said:

can't use results of previous query as arguments in the next query

How do you implement N branching logic without N round trip?

zapov commented 8 years ago

The current implementation does not support that. But I will evolve it into chained CTEs and then it will be able to use result of previous query. I did not improve it as much because I mostly use reports, not this dynamic subqueries.

bhauer commented 8 years ago

My underlying point is that it would be beneficial to this benchmark if it allowed multiple result set implementations since this is normal production usage. It will expose technologies and frameworks which have issues with such a feature.

I understand and the Multi-query test is not that test. At the highest level, it is a proxy for any situation where N round-trips to an external system are necessary. We happen to use a database because that is the most common external service.

I recommend making an argument to the community that a test that uses multiple ResultSets is worthwhile (in issue #133).

We also do not presently accept SQLite implementations despite there being a compelling argument that such an implementation is "production-class." Such an implementation is similarly avoiding the principal work of interacting N times with an external system.

NateBrady23 commented 8 years ago

Resolved with #2249