Properly "inline" nsLDAPServiceEntry

NEW
Unassigned

Status

Directory
LDAP XPCOM SDK
P2
normal
17 years ago
3 months ago

People

(Reporter: Leif Hedstrom, Unassigned)

Tracking

(Blocks: 1 bug)

other
mozilla1.2
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
From Brendans SR= of 70422:

The other followup to-do: really inline these getters (and maybe make them XPCOM
style, if we can't have already_AddRefed; it's incongruous to see an XPCOM-style
out param passed using getter_AddRefs for operation->GetConnection, while the
entry->GetConnection uses the return value).

/be
(Reporter)

Comment 1

17 years ago
I'm also adding these methods to the |nsLDAPServiceEntry|:

|HasMessage()|
|HasConnection()|


and to the |nsILDAPService| interface we're adding

|HasConnection()|


I have a patch for this already, which I'll post here once I've tested it on Mac
and Win32.
Blocks: 17880
Status: NEW → ASSIGNED
Keywords: nsbeta1
(Reporter)

Updated

17 years ago
Priority: -- → P1
(Reporter)

Comment 2

17 years ago
Created attachment 33546 [details] [diff] [review]
First patch
(Reporter)

Comment 3

17 years ago
I've moved all the code for the |nsLDAPServiceEntry| to the |.h| file, and added
the methods mentioned in the bug comment. I still need to test this on Macintoy
(tomorrow).

Dmose, can you review these changes please?

Thanks,

-- Leif

Updated

17 years ago
Target Milestone: --- → mozilla0.9.1

Comment 4

17 years ago
reassign to Olga as QA contact
QA Contact: yulian → olgac

Comment 5

17 years ago
Review comments:

* drop hasConnection, since, as we discussed yesterday, the semantics are not
  what we really want.

* is it really necessary to move all the inline function code into the .h file?
 Would just adding inline to the actual method definitions be sufficient?

* any particular reason not inline nsLDAPServiceEntry::Init?
I dunno about inline in the .cpp files, and I still say these data members
should be exposed to their friends (no snickering).  Care to find out what does
and does not inline?  Anyway, fewer source lines and inlines in the class sound
better to me.  If anyone ever uses one elsewhere, you won't be calling an out of
line copy.

/be

Comment 7

17 years ago
brendan: 

As far as looking at what gets inlined and what doesn't, that seems like
premature optimization to me.  This is exceedingly unlikely to be any sort of
bottleneck, I think.

I'm a little unclear on what exactly you're asking for in the second to last
sentence.  Cutting down on the necessary (inlined) function calls by grovelling
in nsLDAPServiceEntry's members directly?  I'll buy that.  

I assume the last sentence is referring to the fact that |inline| is just a hint
to the compiler, and it might not always happen?
dmose: didn't you read my anti-"patternism" postscript in leif's cvs access
bug?  All these little get and set functions, with their non-XPCOM special
patterns, just obfuscate the code and make it hard to "grep" for thread-unsafe
or deadlock prone call-outs.

Inlining one-line getters used in only one file, if you don't go with exposed
data members (via friend or public) is a premature deoptimization that will
never go away "later", and that compounds linearly in code space and cycles.  Do
this 1000x and then come back asking where performance went, compared to the C
version from four years ago.  No, wait, don't -- that would suck!

/be
(Reporter)

Comment 9

17 years ago
I'm tired, and confused. What exactly are we saying now? Should I not "inline"
these methods in the include file now? Change them to XPCOM style parameter
passing instead?

I thought I understood what was asked for, but I guess not.
leif: I still think moving those functions wholesale into the class decl in the
.h file is the best near-term change.  I also don't see the point in hiding data
from one other file that is friends with it.  But I'm not the boss of you!  Do
what you think is best.

/be
(Reporter)

Comment 11

17 years ago
Ok. So besides removing that new |HasConnection()|, is that patch what you
expect? I didn't inline the |Init()| function, I couldn't think of a reason why
that would be useful.

Thanks,

-- leif
Yeah, don't inline Init.  Also, too much vertical space here:

+    nsLDAPServiceEntry()
+        : mLeases(0),
+          mDelete(PR_FALSE),
+          mRebinding(PR_FALSE)
+
+    {
+        mTimestamp = LL_Zero();
+    }

And you don't need the inline keyword if the method is defined (not just decl'd)
in the class.  Some people prefer one-liner get and set methods, and I do too --
but don't scrunch too much onto one line, ok?

Sorry I didn't review sooner.  At this point, fixing the above nits, sight
unseen by me gets you an sr=, so don't sweat it.

/be

Updated

17 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2

Updated

17 years ago
Priority: P1 → P2
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Updated

17 years ago
Target Milestone: mozilla0.9.3 → mozilla1.0

Updated

17 years ago
Blocks: 104166
(Reporter)

Updated

17 years ago
Target Milestone: mozilla1.0 → mozilla0.9.7
(Reporter)

Comment 13

16 years ago
Dmose
Assignee: leif → dmose
Status: ASSIGNED → NEW

Comment 14

16 years ago
Moving out of 0.9.7.  Feel free to move them past 0.9.8.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Mass change: pushing out 0.9.8 bugs, as I'm concentrating feature work now.
Target Milestone: mozilla0.9.8 → mozilla0.9.9

Updated

16 years ago
Target Milestone: mozilla0.9.9 → mozilla1.2
Assigning bugs that I'm not actively working on back to nobody; use SearchForThis as a search term if you want to delete all related bugmail at once.
Assignee: dmose → nobody
Filter on "Nobody_NScomTLD_20080620"
Assignee: nobody → dmose
QA Contact: olgac → xpcom

Updated

3 months ago
Assignee: dmose → nobody
You need to log in before you can comment on or make changes to this bug.