Author Topic: cc expression methods; passed by value tempoaries seem avoidable  (Read 7525 times)

Offline blauzahn

  • Multiple posting newcomer
  • *
  • Posts: 112
I see no reason why the methods:

Code
ExpressionNode::ParseNodeType(wxString token)

ExpressionNode::IsBinaryOperator(wxString first, wxString second)

Expression::AddToInfixExpression(wxString token)

get their parameters passed by value. To me it looks unnecessary inefficient if used often enough.
Don't we have a whole tree with lots of those?

I recommend to change these to const wxString&.


PS:

It we were in c++11-land, AddToInfixExpression may actually get its parameter passed by value, if
m_InfixExpression is empty most of the time and

Code
m_InfixExpression.push_back(token);

were changed to something like:

Code
m_InfixExpression.emplace_back(std::move(token));

... but that's a different story;-)




Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13393
    • Travis build status
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #1 on: March 09, 2014, 05:16:54 pm »
wxString in wx2.8 has COW semantics, so these are pretty cheap.
But you can always provide a patch, with improvements.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline blauzahn

  • Multiple posting newcomer
  • *
  • Posts: 112
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #2 on: March 09, 2014, 08:45:33 pm »

patch: #003558

wxString in wx2.8 has COW semantics, so these are pretty cheap.
Hasn't that changed with  wx-2.9? And even then, COW is more work than no work.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13393
    • Travis build status
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #3 on: March 09, 2014, 08:57:59 pm »
If you've profiled or benchmarked the code then we can draw conclusions about slow and fast parts.
Everything else is just guessing and people are notoriously known to fail to guess correctly :)
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline killerbot

  • Administrator
  • Lives here!
  • *****
  • Posts: 5295
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #4 on: March 09, 2014, 10:43:45 pm »
the patched version is definitely much better style, lead by example ;-)

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13393
    • Travis build status
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #5 on: March 10, 2014, 12:23:24 am »
We're not talking about style, but performance...
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline killerbot

  • Administrator
  • Lives here!
  • *****
  • Posts: 5295
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #6 on: March 10, 2014, 07:31:03 am »
doesn't matter, is better code anyway, don't depend on whatever wxStrings might optimize or not optimize. Const ref wherever possible.

Offline blauzahn

  • Multiple posting newcomer
  • *
  • Posts: 112
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #7 on: March 10, 2014, 07:50:23 pm »
@obfuscated: I agree with you that measuring is vastly superior to guessing.
And I consider it absolutely critical to make sure that no regressions creep in.
Be it functional or in performance. Thanks for taking that serious. I want my
beloved cb to work after an update, safely and responsive.

This quite tiny change of signature to const& in this case is rather to
increase clarity, showing the intent (query, probably no sink), beware
(myself) of accidentially changing the parameter within the routine ---
but without sacrificing any performance!

No, I haven't gprofed it yet. Do you have some measurements for this case?
Instead I tried for this tiny change to focus on the correctness and even
punched in a good dozen of Unittest++ Checks into a separate file. Just
to make sure I do not break anything (Testing is for whimps only, right?).
Unfortunately, tests for AddToInfixExpression are still to be done because
it is not testable as simple. And I do not consider #ifdef CC_PARSER_TEST
being a sensible automatic unittest strategy.

More effective-C++ ways to be more efficient-C++ (pun intended) can be found
rather elsewhere E.g.:

Code
ExpressionNode::ExpressionNode()
{
    Initialize(wxEmptyString);
}

void ExpressionNode::Initialize(wxString token)
{
    m_UnaryOperator = false;
    m_Token = token;
    m_Type = ParseNodeType(m_Token);
    m_Priority = GetNodeTypePriority(m_Type);
}

All data-member are assigned a value twice when the default-ctor is called
and that is done in an inner loop further down. Did you notice that
the order of the assigments is different from the data-member sequence.
Not that this matters here. But that's how errors creep in.

I wanted to start at the smallest building blocks. Having confidence
in them before I work my way further up.

The real dragons lie are, where the complexity is high (have you fed
pmmcabe with CC?) and local reasoning is hard due to friendship,
inheritance, protected data-member and naked pointers. As for performance:
Did you ever try a sizeof(Token). How many do we have?

I hope, that future replies can be a lot shorter;-)

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 13393
    • Travis build status
Re: cc expression methods; passed by value tempoaries seem avoidable
« Reply #8 on: March 10, 2014, 10:35:29 pm »
You're still guessing! Profile first than make changes, but do not use gprof it will probably be a waste of time.
Use perf, cachegrind, oprofile, vtune or zoom (last two are proprietary)

For testing there is a separate project cctest, I don't know if it is automated or not.
But if you can provide some unittest++ test it won't hurt, probably you can integrate it in the cctest project...
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]