Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/
---

(Updated April 30, 2016, 3:44 a.m.)


Status
--

This change has been discarded.


Review request for KDE Frameworks and Christoph Feck.


Repository: kiconthemes


Description
---

At the moment we're playing Battleship to see if an icon is present in a 
subdirectory. This means that we are checking on every directory if there's an 
icon that matches the size with a said name on every request.

This can be seen easily with strace:
```
$ strace kwrite |& grep ENOENT | wc -l
6212
```
After the patch: 
```
$ strace kwrite |& grep ENOENT | wc -l
1993
```
We reduce these accesses to let QDir keep the list of files inside the 
directory (that was already being generated at some point, it just was being 
discarded).


Diffs
-

  src/kicontheme.cpp 0996054 

Diff: https://git.reviewboard.kde.org/r/127236/diff/


Testing
---

Builds, tests still pass, applications start noticeably faster.


Thanks,

Aleix Pol Gonzalez

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Kevin Funk]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?
> 
> Nick Shaforostoff wrote:
> also for the QFileInfo::lastModified() there may be just not enough time 
> between the tests, so try adding a 2-minute sleep there
> 
> David Faure wrote:
> QFileInfo::lastModified() doesn't include milliseconds yet. There is a 
> pending patch for Qt to implement that (as part of a much bigger task which 
> also adds QFile::setFileTime()), but yeah, that's the problem. Right now you 
> need a 1s sleep whenever unittesting changes to lastModified().
> 
> QTest::qWait(1000); // remove this once lastModified includes ms
> 
> Or at least "up to 1s, until the end of the current second", I have code 
> for that in the kdirwatch unittest for instance.
> 
> Another alternative, hacking the modification time on files (using utime, 
> like I do in ksycocatest.cpp)
> 
> (all this to avoid tests that take 15 seconds in total, I like the policy 
> that a test should run in under 5s -- otherwise people stop running them)
> 
> Nick Shaforostoff wrote:
> so, any news on this one?
> 
> Nick Shaforostoff wrote:
> even simple solution of discarding the cache after certain point of time 
> (e.g. 10 seconds) and going uncached code path would give us the benefit 
> without introducing regression, right?

/me likes that idea. You could make the cache lifetime configurable via 
environment variables, and set that to the minimum for the 
`testUnknownIconNotCached` test.

@Apol: Awesome idea indeed, this reduces a lot of strain on the file system!


- Kevin


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?
> 
> Nick Shaforostoff wrote:
> also for the QFileInfo::lastModified() there may be just not enough time 
> between the tests, so try adding a 2-minute sleep there
> 
> David Faure wrote:
> QFileInfo::lastModified() doesn't include milliseconds yet. There is a 
> pending patch for Qt to implement that (as part of a much bigger task which 
> also adds QFile::setFileTime()), but yeah, that's the problem. Right now you 
> need a 1s sleep whenever unittesting changes to lastModified().
> 
> QTest::qWait(1000); // remove this once lastModified includes ms
> 
> Or at least "up to 1s, until the end of the current second", I have code 
> for that in the kdirwatch unittest for instance.
> 
> Another alternative, hacking the modification time on files (using utime, 
> like I do in ksycocatest.cpp)
> 
> (all this to avoid tests that take 15 seconds in total, I like the policy 
> that a test should run in under 5s -- otherwise people stop running them)
> 
> Nick Shaforostoff wrote:
> so, any news on this one?

even simple solution of discarding the cache after certain point of time (e.g. 
10 seconds) and going uncached code path would give us the benefit without 
introducing regression, right?


- Nick


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?
> 
> Nick Shaforostoff wrote:
> also for the QFileInfo::lastModified() there may be just not enough time 
> between the tests, so try adding a 2-minute sleep there
> 
> David Faure wrote:
> QFileInfo::lastModified() doesn't include milliseconds yet. There is a 
> pending patch for Qt to implement that (as part of a much bigger task which 
> also adds QFile::setFileTime()), but yeah, that's the problem. Right now you 
> need a 1s sleep whenever unittesting changes to lastModified().
> 
> QTest::qWait(1000); // remove this once lastModified includes ms
> 
> Or at least "up to 1s, until the end of the current second", I have code 
> for that in the kdirwatch unittest for instance.
> 
> Another alternative, hacking the modification time on files (using utime, 
> like I do in ksycocatest.cpp)
> 
> (all this to avoid tests that take 15 seconds in total, I like the policy 
> that a test should run in under 5s -- otherwise people stop running them)

so, any news on this one?


- Nick


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[David Faure]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?
> 
> Nick Shaforostoff wrote:
> also for the QFileInfo::lastModified() there may be just not enough time 
> between the tests, so try adding a 2-minute sleep there

QFileInfo::lastModified() doesn't include milliseconds yet. There is a pending 
patch for Qt to implement that (as part of a much bigger task which also adds 
QFile::setFileTime()), but yeah, that's the problem. Right now you need a 1s 
sleep whenever unittesting changes to lastModified().

QTest::qWait(1000); // remove this once lastModified includes ms

Or at least "up to 1s, until the end of the current second", I have code for 
that in the kdirwatch unittest for instance.

Another alternative, hacking the modification time on files (using utime, like 
I do in ksycocatest.cpp)

(all this to avoid tests that take 15 seconds in total, I like the policy that 
a test should run in under 5s -- otherwise people stop running them)


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.
> 
> Nick Shaforostoff wrote:
> there is a difference between changing a file from inside the program and 
> changing it from outside.
> 
> did you make the change externally when testing QFileSystemWatcher-based 
> solution?

also for the QFileInfo::lastModified() there may be just not enough time 
between the tests, so try adding a 2-minute sleep there


- Nick


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before
> 
> Aleix Pol Gonzalez wrote:
> That doesn't work, for some reason...
> ```
>  // Install the existing icon by copying.
> +qDebug() << "1" << 
> QFileInfo(actionIconsDir).lastModified();
>  QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), 
> newIconPath));
> +qDebug() << "2" << 
> QFileInfo(actionIconsDir).lastModified();
> ```
> 
> Both return the same value.

there is a difference between changing a file from inside the program and 
changing it from outside.

did you make the change externally when testing QFileSystemWatcher-based 
solution?


- Nick


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Aleix Pol Gonzalez]

> On March 1, 2016, 5:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?
> 
> David Edmundson wrote:
> Could you compare modified time of mBaseDirThemeDir and reload if needed?
> it'd be one extra stat on the directory each time, but still a saving 
> from before

That doesn't work, for some reason...
```
 // Install the existing icon by copying.
+qDebug() << "1" << QFileInfo(actionIconsDir).lastModified();
 QVERIFY(QFile::copy(QStringLiteral(":/test-22x22.png"), newIconPath));
+qDebug() << "2" << QFileInfo(actionIconsDir).lastModified();
```

Both return the same value.


- Aleix


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 4:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 4:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[David Edmundson]

> On March 1, 2016, 4:37 p.m., Aleix Pol Gonzalez wrote:
> > Eh... I just realized it's not 100% correct. We have a test 
> > (testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.
> > 
> > The problem with this one is that we are not reacting when icons are 
> > introduced at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch 
> > on KIconTheme but it didn't signal.
> > 
> > Should I just discard the patch?

Could you compare modified time of mBaseDirThemeDir and reload if needed?
it'd be one extra stat on the directory each time, but still a saving from 
before


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Aleix Pol Gonzalez]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review93023
---



Eh... I just realized it's not 100% correct. We have a test 
(testUnknownIconNotCached) that fails, unsure how I didn't see it yesterday.

The problem with this one is that we are not reacting when icons are introduced 
at runtime. I tried adding a sneaky QFileSystemWatcher/KDirWatch on KIconTheme 
but it didn't signal.

Should I just discard the patch?

- Aleix Pol Gonzalez


On March 1, 2016, 4:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 4:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Milian Wolff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review92996
---


Ship it!




But I like battleship :)

Awesome work Aleix, I actually looked at that hotspot plenty of times but never 
came up with this elegant version to fix it. Well done!

- Milian Wolff


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Aleix Pol Gonzalez]

> On March 1, 2016, 10:19 a.m., Kai Uwe Broulik wrote:
> > src/kicontheme.cpp, line 675
> > 
> >
> > This looks like it causes a lot of transient allocations, creating a 
> > QList just to do a contains() on it. Or is this list kept/cached by QFile?

The list is cached by QDir. See there's a `QStringList QDirPrivate::files` 
defined in qdir_p.h.


- Aleix


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review92976
---


On March 1, 2016, 4:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 4:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Kai Uwe Broulik]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review92976
---




src/kicontheme.cpp (line 675)


This looks like it causes a lot of transient allocations, creating a QList 
just to do a contains() on it. Or is this list kept/cached by QFile?


- Kai Uwe Broulik


On März 1, 2016, 3:25 vorm., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated März 1, 2016, 3:25 vorm.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Ralf Habacker]

Am 01.03.2016 um 09:16 schrieb Nick Shaforostoff:

> At the moment we're playing Battleship to see if an icon is present in
> a subdirectory. This means that we are checking on every directory if
> there's an icon that matches the size with a said name on every request.
>
>
> This can be seen easily with strace:
>
>
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
>
>
> After the patch:
>
>
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
>
Nice work :-)

This is one of the major KF5 issues on Windows.

Ralf

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127236: Leverage QDir to know what's in a KIconThemeDir

[Nick Shaforostoff]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127236/#review92967
---


Ship it!




great finding, thanks for you work!

- Nick Shaforostoff


On March 1, 2016, 3:25 a.m., Aleix Pol Gonzalez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127236/
> ---
> 
> (Updated March 1, 2016, 3:25 a.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> ---
> 
> At the moment we're playing Battleship to see if an icon is present in a 
> subdirectory. This means that we are checking on every directory if there's 
> an icon that matches the size with a said name on every request.
> 
> This can be seen easily with strace:
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 6212
> ```
> After the patch: 
> ```
> $ strace kwrite |& grep ENOENT | wc -l
> 1993
> ```
> We reduce these accesses to let QDir keep the list of files inside the 
> directory (that was already being generated at some point, it just was being 
> discarded).
> 
> 
> Diffs
> -
> 
>   src/kicontheme.cpp 0996054 
> 
> Diff: https://git.reviewboard.kde.org/r/127236/diff/
> 
> 
> Testing
> ---
> 
> Builds, tests still pass, applications start noticeably faster.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel