MySQL Lists are EOL. Please join:

List:Internals« Previous MessageNext Message »
From:Masood Mortazavi Date:April 14 2009 7:42am
Subject:Re: Const propagation within a non-const expression
View as plain text  
Hi Kazuho -

  Given Timour's suggestions, may I suggest that you move this patch
to bugs DB for better tracking: http://bugs.mysql.com/

  This way, you can dispatch each of Timour's suggestions there and
make tracking easier.

Regards,
Masood

P.S.
You can post the relevant bug number / url on this thread, and post
the url for this  thread in the comment section of the bugs db.
Yes, we need better e-mail / issue tracking integration ... :-)


On Thu, Apr 2, 2009 at 8:07 AM, Timour Katchaounov
<Timour.Katchaounov@stripped> wrote:
> Hi Kazuho,
>
> Thank you very much for your contribution.
>
> I applied and tested your patch
>  http://q4m.31tools.com/misc/mysql-subexp-const-prop.patch
> with the current 6.0 tree being the latest reasonably stable development
> tree.
> Version 5.1 is not an option because in order to avoid possible
> destabilization
> we do not add new features to it (even more so optimizer features).
>
> The patch has a number of issues described below. Please notice that in this
> review I applied the same approach as we normally do for internal patches.
> The general requirements for all contributions to MySQL are outlined here:
> http://forge.mysql.com/wiki/Contributing_Code
>
> 1. Issues that are visible immediately
> --------------------------------------
>
> 1.1 Compilation
> When building with your patch, GCC produces a number of related warnings:
> item_cmpfunc.cc: In member function ‘int
> Arg_comparator::set_cmp_func(Item_bool_func2*, Item**, Item**,
> Item_result)’:
> item_cmpfunc.cc:913: warning: enumeration value ‘FIELD_ITEM’ not handled
> in
> switch
> item_cmpfunc.cc:913: warning: enumeration value ‘SUM_FUNC_ITEM’ not
> handled
> in switch
> item_cmpfunc.cc:913: warning: enumeration value ‘STRING_ITEM’ not handled
> in
> switch
> ...... and so on ...
>
> 1.2 Regression tests
> Running the regression test for a debug build results in a number of failing
> tests. I ran the tests for a binary built with the command
> "./BUILD/compile-pentium-debug-max-no-ndb". The command to run the test was:
> "./mysql-test-run.pl  --force [--ps-protocol] --mem --skip-rpl
> --max-test-fail=100".
> Notice that we run tests both with and without --ps-protocol. The majority
> of
> the failing tests are related to collations, these are tests with the prefix
> "ctype".
>
> This is a complete list of the failing tests:
>  binlog.binlog_killed main.ctype_latin1_de main.binary main.ctype_collate
>  main.ctype_ucs main.ctype_utf16 main.ctype_utf32 main.distinct main.range
>  main.row main.sp main.crash_commit_before
>
> 1.3 Design
> This is a new feature. As such it should have at least some design
> description.
> In your case, I would expect to see a description of what queries/conditions
> will be optimized, and which ones will *not* be optimized. Such analysis
> should
> also lead to a check for the applicability of the optimization. It is also
> important to consider whether the optimization always leads to a performance
> improvement, or if there are cases when performance may degrade.
>
> The current description of the patch says:
> "MySQL (5.1.23-rc) does not do const-propagation of a subexpression which is
> part
> of a non-const expression". However, the patch itself is not related to
> constant propagation - no constants are being propagated (substituted) at
> all,
> thus your comment is a bit misleading.
>
> 1.4 Feature regression test
> There is no test specific for the patch. If one hasn't thought of the class
> of queries covered by some optimization, it will be hard to design a test
> for
> that optimization. When working on a test, make sure that everything works
> also with prepared statements. One needs to be careful with prepared
> statements
> because certain optimizations are not (cannot be) remembered after prepare
> (or
> after re-execution).
>
>
> 2. Issues with the approach
> ----------------------------
> 2.1 Collations
> From the test above it is clear that the patch doesn't take into account
> collations.
>
> 2.2 Design
>
> * Looking at the patch itself, I think that the idea to cache the value
>  is not the best way to solve the problem. Why not substitute the expression
>  with its value? Look at propagate_cond_constants() in sql_select.cc.
>  I am not convinced that Arg_comparator::set_cmp_func() is the right
>  place for such a transformation.
>
> * The approach seems pretty limited (I would say "hacky") - why do
>  the transformation only in Arg_comparator::set_cmp_func()? Why
>  not substitute constant expressions elsewhere, e.g. in all function
>  arguments? Or in general? I suggest looking at how the current constant
>  substitution propagation works, and apply that approach instead.
>
> Let me know if you would like to address the issues above. I understand
> that this is a lot more work than the small patch you sent. Please notice
> that I haven't thought in detail how to really implement a more general
> constant substitution, so I don't have a ready answer how to do it.
>
>
> Best regards,
> Timour
>
>
>> Hi,
>>
>> I just noticed that MySQL (5.1.23-rc) does not do const-propagation of
>> a subexpression which is part of a non-const expression, and wrote a
>> tiny patch (attached) for the situation.
>>
>> For example,
>>
>>     where v<(1>0)
>>     where v=func(3)
>>     where v=if(0,1,2)
>>
>> Of these expressions, my patch checks if either side of the
>> comparators are constant expressions, and if they are constants (and
>> if they are either COND_ITEM or FUNC_ITEM), caches the value as a
>> constant.
>>
>> The patch does not cache the information if both side of the
>> comparators are constants, since the value would be cached at a higher
>> level, such as the caller of the call tree or by a call to
>> remove_eq_conds.
>>
>> Would there be any chance for such a patch to be accepted to your
>> source tree?  Are there any advises or suggestions?
>>
>> Thank you in advance.
>>
>
>
> --
> MySQL Internals Mailing List
> For list archives: http://lists.mysql.com/internals
> To unsubscribe:
>  http://lists.mysql.com/internals?unsub=1
>
>
Thread
Const propagation within a non-const expressionKazuho Oku28 Feb
  • Re: Const propagation within a non-const expressionKazuho Oku28 Feb
  • Re: Const propagation within a non-const expressionTimour Katchaounov2 Apr
    • Re: Const propagation within a non-const expressionMasood Mortazavi14 Apr
  • Re: Const propagation within a non-const expressionTimour Katchaounov2 Apr