Last Comment Bug 850466 - do some trivial optimisations in mailnews/base/utils/folderUtils.jsm
: do some trivial optimisations in mailnews/base/utils/folderUtils.jsm
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 22.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-12 15:48 PDT by :aceman
Modified: 2013-03-15 10:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.24 KB, patch)
2013-03-12 15:49 PDT, :aceman
mkmelin+mozilla: review+
neil: feedback+
Details | Diff | Splinter Review
patch v2 (4.26 KB, patch)
2013-03-13 10:39 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2013-03-12 15:48:21 PDT
These functions are called for all folders in the folder pane repeatedly, e.g. when the mouse is going over them or scrolling. So if the changes in the patch speed them up, it should be good.
Comment 1 :aceman 2013-03-12 15:49:45 PDT
Created attachment 724176 [details] [diff] [review]
patch
Comment 2 neil@parkwaycc.co.uk 2013-03-12 17:27:13 PDT
Comment on attachment 724176 [details] [diff] [review]
patch

As far as I know, SeaMonkey doesn't use these functions, so maybe mkmelin can review instead?

>+  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
>+  const flags = aFolder.flags;
I don't like using const for values that change between function calls.
Comment 3 :aceman 2013-03-13 01:52:33 PDT
Neil, can you tell if the 'const flags = aFolder.flags' actually saves anything? I.e. if the real value of aFolder.flags is stored into 'flags' so that further uses of 'flags' do not reach down to the nsIMsgFolder and retrieve the flags value again from C++? Or is that just a link (alias, pointer) and it does not save anything?
Comment 4 Magnus Melin 2013-03-13 05:33:39 PDT
Comment on attachment 724176 [details] [diff] [review]
patch

Review of attachment 724176 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=mkmelin

::: mailnews/base/util/folderUtils.jsm
@@ +18,5 @@
>   * @param aFolder  the nsIMsgFolder whose special type should be returned
>   */
>  function getSpecialFolderString(aFolder) {
> +  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
> +  const flags = aFolder.flags;

yes, please don't use const for non-constants. (let instead)

aFolder.flags crosses the xpconnect bounderies, using the local variable caches that (if i understand it correctly).

@@ +78,5 @@
>      properties.push("newMessages-true");
>  
> +  if (aFolder.isServer) {
> +    properties.push("isServer-true");
> +  } else {

i'd prefer elses on new line
Comment 5 :aceman 2013-03-13 05:50:58 PDT
(In reply to Magnus Melin from comment #4)
> >  function getSpecialFolderString(aFolder) {
> > +  const nsMsgFolderFlags = Components.interfaces.nsMsgFolderFlags;
> > +  const flags = aFolder.flags;
> 
> yes, please don't use const for non-constants. (let instead)
For the flags variable or both of them?

> aFolder.flags crosses the xpconnect bounderies, using the local variable
> caches that (if i understand it correctly).
That would be what I want to achieve.
Comment 6 Magnus Melin 2013-03-13 06:03:09 PDT
nsMsgFolderFlags is kind of a constant so that should be a const
Comment 7 :aceman 2013-03-13 10:39:14 PDT
Created attachment 724493 [details] [diff] [review]
patch v2

OK, I've put a printf into nsMsgDBFolder::GetFlags and I see there are much less calls of the function now, so the caching works.
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-03-15 10:40:43 PDT
https://hg.mozilla.org/comm-central/rev/bbf12a153816

Note You need to log in before you can comment on or make changes to this bug.