Bug 250676

Summary: Use memcpy in SVGPathByteStreamSource::readType
Product: WebKit Reporter: Ahmad Saleem <ahmad.saleem792>
Component: SVGAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: sabouhallawa, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   

Description Ahmad Saleem 2023-01-16 05:24:17 PST 
Hi Team,

While going through Blink's commit, I came across following refactoring / optimization:

Blink Commit - https://chromium.googlesource.com/chromium/blink/+/f9411437595a5782eb1adba1f719d4448530bd93

WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/svg/SVGPathByteStreamSource.h#57

'''

This avoids having the compiler beating around the bush trying to
recognize the memcpy, and results in smaller code (roughly 50% smaller
measured for GCC 4.8). All at the affordable price of upsetting a few
language purists.

'''

Appreciate if someone can share input.

Thanks!
Comment 1 Radar WebKit Bug Importer 2023-01-23 05:25:16 PST 
<rdar://problem/104552389>
Comment 2 Said Abou-Hallawa 2023-01-24 12:41:01 PST 
I think there is no much saving in the Blink's commit. This function just reads bool, short or float. So the for-loop in SVGPathByteStreamSource::readType() runs 1, 2 or 4 times. In my opinion using memcpy is just a cleaner solution and nice refactoring but it is not a perf or code size optimization.


So if we want to clean this area, I would suggest something like that instead:


    // SVGPathByteStreamSource.h

    template<typename DataType>
    DataType readType()
    {
        DataType data;
        size_t dataSize = sizeof(DataType);

        ASSERT(m_streamCurrent + dataSize <= m_streamEnd);
        memcpy(&data, m_streamCurrent, dataSize);
        m_streamCurrent += dataSize;
        return data;
    }


    // SVGPathByteStreamBuilder.h

    template<typename DataType>
    void writeType(DataType data)
    {
        union {
            DataType value;
            unsigned char bytes[sizeof(DataType)];
        } ByteType;

        ByteType type = { data };
        size_t typeSize = sizeof(ByteType);

        for (size_t i = 0; i < typeSize; ++i)
            m_byteStream.append(type.bytes[i]);
    }


And we can get rid of BoolByte, FloatByte and UnsignedShortByte.
Comment 3 Chris Dumez 2023-08-30 19:47:12 PDT 
Pull request: https://github.com/WebKit/WebKit/pull/17264
Comment 4 EWS 2023-08-31 14:46:49 PDT 
Committed 267523@main (e2aa0c20fc47): <https://commits.webkit.org/267523@main>

Reviewed commits have been landed. Closing PR #17264 and removing active labels.