FWD » Core Development » Database

Review of 9756a/15766.

• in FqlToSqlConverter, the fqlAstCache is static, and the key is just the fql. This is not correct. The current solution will work only on simple cases. Think of two queries on DIFFERENT databases (maybe different buffers of same database?) which apparently request apparently the same data from SQL, but the table structure are different. The solution is to create a key for fqlAstCache which contains the DMO classes as well. Their paths contain enough information to distinct tables from different databases, even if they have the same name;
• also in FqlToSqlConverter.fqlAstCache, I think there are 90% cases when the AST tree does not require to be duplicate() -d when added to cache or extracted. That is because the FQLAst root is usually only processed locally, in toSQL() and does not 'leak'. For cases when the instance is created and preprocess() -ed, we can add an annotation to mark 'mutable' trees in case they are affected by processStatement(). I think this happens only for word-indexed instances in case of SELECT statements. This is just an assumption and need to be double checked;
• related to the new, rather large, boolean arrays in Contains and LogicalExpressionConverter. I think we can replace these occurrences with BitSet. These are pretty fast data structures and use the memory optimally (1 bit per boolean), in a dynamic manner (allocating memory only for set places);
• in Functions.getDelimiterPattern() and LogicalExpressionConverter$Lexer(), the compiled patterns are simplified. Also in the latter case skipPatterns is static and NOT synchronised! At least not evidently. I looked up the call stack a few levels and I couldn’t spot any.

Ovidiu Maxiniuc wrote:

Review of 9756a/15766.

I agree that these are two directions we can improve performance. The ideas are good. But I think there are a few issues to discuss:

• in FqlToSqlConverter, the fqlAstCache is static, and the key is just the fql. This is not correct. The current solution will work only on simple cases. Think of two queries on DIFFERENT databases (maybe different buffers of same database?) which apparently request apparently the same data from SQL, but the table structure are different. The solution is to create a key for fqlAstCache which contains the DMO classes as well. Their paths contain enough information to distinct tables from different databases, even if they have the same name;

The cache I made only saves the ast given by FQLParser, and has only the String fql as argument. So, without any other processing.

Ovidiu Maxiniuc wrote:

• also in FqlToSqlConverter.fqlAstCache, I think there are 90% cases when the AST tree does not require to be duplicate() -d when added to cache or extracted. That is because the FQLAst root is usually only processed locally, in toSQL() and does not 'leak'. For cases when the instance is created and preprocess() -ed, we can add an annotation to mark 'mutable' trees in case they are affected by processStatement(). I think this happens only for word-indexed instances in case of SELECT statements. This is just an assumption and need to be double checked;

Hmm, I think that the cache only needs to be made for queries with CONTAINS because for these queryWasRewritten would be true and because of this the cache used in Query.createSqlQuery is not used.

Ovidiu Maxiniuc wrote:

• related to the new, rather large, boolean arrays in Contains and LogicalExpressionConverter. I think we can replace these occurrences with BitSet. These are pretty fast data structures and use the memory optimally (1 bit per boolean), in a dynamic manner (allocating memory only for set places);

boolean[] is fastest with some memory compromise indeed. I will also test with BitSet.

• in Functions.getDelimiterPattern() and LogicalExpressionConverter$Lexer(), the compiled patterns are simplified. Also in the latter case skipPatterns is static and NOT synchronised! At least not evidently. I looked up the call stack a few levels and I couldn’t spot any.

Oh, right skipPatterns should have been a ConcurrentHashMap.

Do you have anything else to add ?

Stefanel Pezamosca wrote:

The cache I made only saves the ast given by FQLParser, and has only the String fql as argument. So, without any other processing.

That's right. I've been deceived when comparing the revisions. Scratch the first point. And seconds, as well. Unless we extend the cache to processed trees.

boolean[] is fastest with some memory compromise indeed. I will also test with BitSet.

That's true. But BitSet is optimised for accesses, so I think the trade is worth it.

• in Functions.getDelimiterPattern() and LogicalExpressionConverter$Lexer(), the compiled patterns are simplified. Also in the latter case skipPatterns is static and NOT synchronised! At least not evidently. I looked up the call stack a few levels and I couldn’t spot any.

Oh, right skipPatterns should have been a ConcurrentHashMap.

Correct. I was thinking of external synchronisation. Switching to ConcurrentHashMap should do it.

Do you have anything else to add ?

Not at this time :).

I changed boolean[] to BitSet and used ConcurrentHashMap for skipPatterns in 9756a revision 15767.

Cool. I have looked at the code. The changes are minimal. They are OK in my opinion. However, we can assume the first 128 characters (base Latin set) have high probabilities to be processed so we can pre-allocate them by using new BitSet(128).

At the same time, please redo the original 100000-iterations test from the 1st note and post the results.

Oh, I discovered an issue that is also in trunk. One example of problematic statement contains(words, -) I will fix this in 9756a.

I fixed this in 9756a revision 15769.

Other tests passed. If there are no other objections I think 9756a is safe for trunk.

I have continued testing various CONTAINS testcases and there were no other issues.
This can be merged.

Branch 9756a was merged into trunk as rev. 15784 and archived.