On Mon, Mar 10, 2014 at 5:02 PM, Felipe Magno de Almeida
<felipe.m.alme...@gmail.com> wrote:
> Hello Gustavo,
>
> On Mon, Mar 10, 2014 at 12:45 PM, Gustavo Sverzut Barbieri
> <barbi...@gmail.com> wrote:
>> Cedric,
>>
>> while I agree with performance concerns, nobody is doing core parts in
>> C++ (yet), so for the random app user this is negligible performance
>> hit. Of course C++ mixes nicely with C, so it is still possible to use
>> the original macros/functions which will execute vfprintf() only if
>> log level is required.
>>
>> Felipe, could we make the operator implementations just return if
>> current log level is not to be used? Like, in the beginning of the
>> operator we could do:
>>
>>     if (eina_log_domain_registered_level_get(mydom.id) >
>> EINA_LOG_LEVEL_DBG) return;  // used for mydom.dbg "io";
>
> One problem is that we can't stop the evaluation of function calls, so
> for example:
>
> err << "hello world " << expensive_call();
>
> expensive_call is evaluated before the operator overload runs. By
> that time, it doesn't really matter anymore.

that doesn't change anything from what we have in C now. If you do say
DBG("%s", expensive_call()) it will happen as well. UNLESS you do the
--with-maximum-log-level, then you fall in the constant < constant
evaluation and the compiler removes the whole block.

that's why we have the eina_log_domain_registered_level_get(). I often
use this to avoid the expensive calls (let's say converting a struct
to string using strbuf or walking a hash/list). This way we keep it
dynamic (unlike maximum-log-level) and allows to avoid expensive
calls.



> Also, the check runs
> multiple times for each operator<< function call.

doesn't matter this is super-cheap. One could also save the actual
Eina_Log_Domain structure and check ->level directly. But I don' t
think it is required.


> Also, with the positive case for logging we must share
> std::stringstream's or create one for each operator<< call. Both
> have important disadvantages, for the first we need synchronization,
> for the second we don't get amortized dynamic allocations. It is
> already bad that I can't steal the std::string from std::stringstream,
> which means an unnecessary copy.

that's an implementation detail I'd say. You can create your own
version of stringstream that you can keep reusing, even by using
eina_strbuf internally.

IMO this would be a bit different than what you propose, which makes
it simpler. You'd need a lock, sure, but this is the case with eina
log as well, so much of the same.

You create a stream for each domain log level. That's why I'm saying
efl::eina::log::domain::level_as_stream. This is not what you propose
as using just domain and the macro evaluates the level. By using a
"level_as_stream" you can keep a single stringstream, every "<<"
operation you append to that stringstream. Upon termination (ie:
"endl"), you use the contents of stringstream and reset it to
length=0.


> Since I can't stop evaluation of expressions without macros here,
> it can have a significant impact on performance.

it is the same as in c... as I said above.



>> that way the performance impact is negligible. AFAIR the evaluation is
>> left->right, thus the following could be grouped as:
>>
>>     mydom.dbg << "hello world" << 0.3456;
>>    (mydom.dbg << "hello world") << 0.3456;
>>
>> which usually the operator<<() would return an instance of itself,
>> then it results in:
>>
>>    mydom.dbg << "hello world" << 0.3456;
>>    mydom.dbg << "hello world";  mydom.dbg << 0.3456;
>>
>> so if operator<<() does the level check (which is very light), then we
>> get performance and usability.
>
> For printing constants that is true, but not negligible for expressions
> that are expensive to evaluate.
>
> What I *may* do is: (without impacting performance)
>
> EINA_CXX_DOM_LOG_CRIT(domain) << "hello world " << 0.3456;
>
> But I'm not 100% sure.

I disagree with EINA_CXX_DOM_LOG_CRIT(). You just need a macro to get
__FILE__, __FUNCTION__ and __LINE__. Then the following is better:

    EINA_CXX_LOG(domain.crit) << "hello world" << 0.3456;

domain is a class instance and "crit" is member providing another
instance that implements a stream-like iface. Then EINA_CXX_DOM_LOG()
could be like:

   #define EINA_CXX_LOG(stream) \
      stream.start(__FILE__, __FUNCTION__, __LINE__)


[note: start() may be a horrible name, feel free to change that.]

With stream.start(const char *file, int line, const char *funcname)
returning "this" after setting those internally:

    stream start(const char *file, const char *funcname, int line) {
       this.file = file; /* assume std::string or something that gets
memory right *'/
       this.funcname = funcname;
       this.line = line;
       return this;
    }

Then operator<<() is something like:

    stream operator<<(const char *string) {
       if (eina_log_domain_registered_level_get(this.dom) <this.level)
          return this;

       lock();
       if (string == endl) {
           eina_log_print(this.dom, this.level, this.file,
this.funcname, this.line, "%s", this.message);
           this.message.reset();
       } else {
           this.message.append(string);
       }
       unlock();
       return this;
    }


> Even with expression templates I can't stop the evaluation of the
> expressions, because it is its results that are passed to the
> operator<< function call as arguments.


same as in C :-D


-- 
Gustavo Sverzut Barbieri
--------------------------------------
Mobile: +55 (19) 99225-2202
Contact: http://www.gustavobarbieri.com.br/contact

------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel

Reply via email to