[poppler] poppler/Array.cc poppler/Dict.cc

Wed May 9 14:22:46 UTC 2018

Hello,

I did a text-output perftest against the current master and the result
is almost favorable towards the STL-based Dict implemenation:

54638 matching result(s):
        Cumulative run time:
                Result: 13.06 min ∓ 1.8 %
                Reference: 13.18 min ∓ 1.7 %
                Deviation: -0.01 %
        Cumulative memory usage:
                Result: 797.7 MB %
                Reference: 807.9 MB %
                Deviation: -0.01 %
2654 matching result(s) above the given threshold of 5.0 %:
        Cumulative run time:
                Result: 0.92 min ∓ 1.8 %
                Reference: 0.95 min ∓ 1.8 %
                Deviation: -0.03 %
        Cumulative memory usage:
                Result: 40.2 MB %
                Reference: 40.8 MB %
                Deviation: -0.02 %

Best regards,
Adam.

Am 08.05.2018 um 19:26 schrieb Adam Reichold:
> Hello again,
> 
> sorry for the rapid succession of patches.
> 
> The attached version fixes a linker issue due to non-exported weak
> symbols and fixes all users of Dict::add to not call copyString and leak
> memory since std::string will take care of this including SBO.
> 
> Best regards, Adam.
> 
> P.S.: Will try to do some performance comparisons against master but
> this will take at least a day due to the limitations of my machine.
> 
> Am 08.05.2018 um 08:23 schrieb Adam Reichold:
>> Also cleaned up the includes a bit...
>>
>> Am 08.05.2018 um 08:01 schrieb Adam Reichold:
>>> Hello again again,
>>>
>>> Am 08.05.2018 um 07:37 schrieb Adam Reichold:
>>>> Hello again,
>>>>
>>>> Am 08.05.2018 um 00:12 schrieb Albert Astals Cid:
>>>>> El dilluns, 7 de maig de 2018, a les 23:03:00 CEST, Adam Reichold va escriure:
>>>>>> Hello,
>>>>>>
>>>>>> it is probably already too much purely technical code churn for this
>>>>>> release cycle, but the use of memmove got me thinking how an
>>>>>> implementation of Dict with more STL usage would look like.
>>>>>>
>>>>>> The result is attached, requesting comments on whether this is
>>>>>> considered useful or not.
>>>>>
>>>>> The remove implementation is wrong since it breaks sorting.
>>>>
>>>> Could you explain in more detail? My thinking was that any sublist of a
>>>> sorted list is also sorted and hence removing an element of a list
>>>> cannot break its sorting? (And since we do not increase the size by
>>>> removing an element, we cannot reach SORT_LENGTH_LOWER_LIMIT if we were
>>>> not there already.)
>>>
>>> Sorry, that took a minute... Because of the swap... But actually, there
>>> already is std::vector::erase which does all the tricks like using
>>> memmove if it is statically safe, so it should actually be simpler.
>>>
>>> Fixed version attached...
>>>
>>>> Best regards,
>>>> Adam
>>>>
>>>>> Cheers,
>>>>>   Albert
>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Adam
>>>>>>
>>>>>> Am 07.05.2018 um 19:14 schrieb Albert Astals Cid:
>>>>>>>  poppler/Array.cc |    2 +-
>>>>>>>  poppler/Dict.cc  |    2 +-
>>>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> New commits:
>>>>>>> commit 07b8f838b3d8859a3ad34a3140bb24475bd6ac2c
>>>>>>> Author: Albert Astals Cid <aacid at kde.org>
>>>>>>> Date:   Mon May 7 19:13:07 2018 +0200
>>>>>>>
>>>>>>>     cast to void * to bypass new gcc -Wclass-memaccess warning
>>>>>>>     
>>>>>>>     Yes what we're doing there is a bit nasty but it works :D
>>>>>>>
>>>>>>> diff --git a/poppler/Array.cc b/poppler/Array.cc
>>>>>>> index 3276349f..e8aa34ea 100644
>>>>>>> --- a/poppler/Array.cc
>>>>>>> +++ b/poppler/Array.cc
>>>>>>> @@ -112,7 +112,7 @@ void Array::remove(int i) {
>>>>>>>
>>>>>>>  #endif
>>>>>>>  
>>>>>>>    }
>>>>>>>    --length;
>>>>>>>
>>>>>>> -  memmove( elems + i, elems + i + 1, sizeof(elems[0]) * (length - i) );
>>>>>>> +  memmove( static_cast<void*>(elems + i), elems + i + 1, sizeof(elems[0])
>>>>>>> * (length - i) );> 
>>>>>>>  }
>>>>>>>  
>>>>>>>  Object Array::get(int i, int recursion) const {
>>>>>>>
>>>>>>> diff --git a/poppler/Dict.cc b/poppler/Dict.cc
>>>>>>> index a431f7eb..bc86fd77 100644
>>>>>>> --- a/poppler/Dict.cc
>>>>>>> +++ b/poppler/Dict.cc
>>>>>>> @@ -201,7 +201,7 @@ void Dict::remove(const char *key) {
>>>>>>>
>>>>>>>        gfree(entries[pos].key);
>>>>>>>        entries[pos].val.free();
>>>>>>>        if (pos != length) {
>>>>>>>
>>>>>>> -        memmove(&entries[pos], &entries[pos + 1], (length - pos) *
>>>>>>> sizeof(DictEntry)); +        memmove(static_cast<void*>(&entries[pos]),
>>>>>>> &entries[pos + 1], (length - pos) * sizeof(DictEntry));> 
>>>>>>>        }
>>>>>>>      
>>>>>>>      }
>>>>>>>    
>>>>>>>    } else {
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> poppler mailing list
>>>>>>> poppler at lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> poppler mailing list
>>>>> poppler at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> poppler mailing list
>>>> poppler at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> poppler mailing list
>>>> poppler at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
>>>>
>>>>
>>>> _______________________________________________
>>>> poppler mailing list
>>>> poppler at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/poppler
>>>>
>>>>
>>>> _______________________________________________
>>>> poppler mailing list
>>>> poppler at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/poppler

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/poppler/attachments/20180509/436f71e5/attachment.sig>