[Webkit-unassigned] [Bug 27511] Add WinCE specific platform/graphics files to WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27511





--- Comment #60 from Yong Li <yong.li at torchmobile.com>  2009-08-09 06:36:50 PDT ---
(In reply to comment #59)
> (In reply to comment #58)
> > > > +struct jpeg_dest_mgr {
> > > > +    struct jpeg_destination_mgr pub;
> > > > +    Vector<char> buffer;
> > > > +    Vector<char>* dump;
> > > > +};
> > > 
> > > Naming style (here and below)
> > 
> > I see { should be in the next line here. I'm told that "m_" should not be used
> > for "struct"
> 
> Based on http://webkit.org/coding/coding-style.html , the struct name should be
> CamelCase (with the first letter capitalized), and the members should have m_
> (I don't see an exception for structs and haven't heard this before).  Also the
> names should be more descriptive than "dest_mgr" or "pub"; use full words where
> possible.
> 
> > > Also, the 2k capacity value seems arbitrary.  How do we know that that's a good
> > > value?  Why not just:
> > >     Vector<JSAMPLE> rowBuffer(compressData.image_width * 3);
> > > ?
> > try to benefit from inline vector. if total bytes of a row is less than 2k, we
> > don't need to allocate memory from heap.
> 
> I haven't used this technique with the WTF library so I'm pretty ignorant of
> its implementation detail.  If you don't declare the 2k capacity, is the vector
> guaranteed to heap-allocate no matter the size, or would it still do something
> smart for you under the hood?  How was 2k picked here -- was it arbitrary, or
> based on some data?  Is there a downside other than stack usage?  Comments
> would be helpful.
> 
Heap allocation is slower than stack allocation. 2K is arbitrary.

> > > > +            for (const unsigned short* rowEnd = pixel + compressData.image_width; pixel < rowEnd;) {
> > > > +                unsigned short value = *pixel++;
> > > 
> > > Might be a little clearer to do:
> > > for (...; ...; ++pixel) {
> > >     unsigned short value = *pixel;
> > 
> > *foo++ is very popular in image processing code. Not sure if it can help
> > compiler optimization.
> 
> I cannot speak with perfect authority, but I did spend five years as a compiler
> engineer, and my impression is that your code is if anything less likely to be
> optimized than my suggestion.  (It's very likely the two generate the same
> code.)
> 
> It would be awesome if you had some sort of testcase that exercised a lot of
> this code in a way that could be timed.  Of course benchmarking inner loops is
> tricky given the effects of processor caches and the like, but such a thing
> would be immensely valuable in the future, where I believe we'll try to
> rearchitect the Image system to include the capabilities you need more
> directly, instead of as something of a fork (like you've had to maintain until
> now).  It would help reassure me that, for example, replacing some particular
> pixel-writing loop with a call to an inlined function did not actually hurt
> performance.  Otherwise, the best we can do is probably manual assembly
> inspection which is pretty error-prone when determining real perf.

In the case that compiler doesn't optimize, *foo++ may result this kind of
machine code 
1) move foo to register
2) save *foo to register or some variable
3) increase the register by one that keeps foo
4) save the register back to stack

But if ++foo is called later, then = *foo moves foo to register once, and later
++foo does that again.

Of course, most compilers may be smart enough to optimize this when they find
foo is not used in-between *foo and ++foo. Long time ago, it was said smaller
C/C++ code could result fast machine code. This may be the reason why there's
"++" operator in C. Because "foo += 1" is even more readable.

But the point is not that, I have no problem with choosing either one. I just
think it may be a time waste on such small things. Each C++ programmer has his
personal programming style, because everyone has different mind. I agree with
that some basic coding style rules do help, but too many brainwashing things
just reduce the fun of C++ programming and waste time. I believe that you would
write very different code if you implemented it. If the reviewer was me,
probably you would be asked to change *foo and ++foo to *foo++.

> 
> > > > +        for (; pixel < pixelEnd; ) {
> > > 
> > > I find "while (pixel < pixelEnd) {" clearer.
> > 
> > Seems webkit prefer "while" to "for". Somehow I started hating "while" a few
> > years ago.
> 
> I think I'm a bigger fan of for() (over while()) than most, since I tend to get
> nitpicky about minimizing variable scopes.  But in the case where there is no
> index variable declared and no iteration clause, while() means precisely what
> you're doing, whereas for() just results in adding ; ; and making things more
> opaque.  Precision in syntax is a virtue...

In this case, I agree that while() is better. But what's your opinion about
"while(true)" vs. "for(;;)"?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.