Closed Bug 80364 Opened 24 years ago Closed 6 years ago

xpconnect could save a little work in iterating the foundDependentParam case

Categories

(Core :: XPConnect, defect, P3)

defect

Tracking

()

RESOLVED INACTIVE
mozilla1.7alpha

People

(Reporter: jband_mozilla, Assigned: dbradley)

Details

Attachments

(1 file)

shaver points out... > > + if(foundDependentParam) > + { > + for(i = 0; i < paramCount; i++) > > So instead of just using a boolean, you could store the index of the first > dependent param, and save yourself some iterations, no? (Not that the > !IsDependent cases are very expensive, of course.) > > + for(i = 0; i < paramCount; i++) > + { > + const nsXPTParamInfo& paramInfo = methodInfo->GetParam(i); > + if(!paramInfo.IsOut() && !paramInfo.IsDipper()) > + continue; > > Likewise for storing the first out/dipper param index and starting there? > > +done: > + // iterate through the params (again!) and clean up > + // any alloc'd stuff and release wrappers of params > + if(dispatchParams) > + { > > Ditto for storing the first param for cleanup? This is pretty trivial but a valid point.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
Moving up for performance/footprint focus
Target Milestone: mozilla1.0 → mozilla0.9.7
Priority: -- → P3
The benefit of this is pretty trivial moving out past 1.0. I'm attaching a patch that I mucked around with. It eliminates the use of the dependent param loop. I had three goals, speed things up, break CallMethod into smaller pieces, and reduce code size. I was able to achieve two of the three, code size didn't decrease, but increased slightly. I was hoping with the removal of the loop code I'd see a small gain.
Target Milestone: mozilla0.9.7 → mozilla1.0.1
dbradley: This bug is about suggesting an optimization with near zero value (few methods have dependent params AND many params AND would be sped up in any measurable way by skipping iterations in a quickly shorcircuited loop). Yet, you are suggesting a 1200 line patch that completely reorganizes the code and introduces an unknown number of potential bugs. e.g. check the meaning of IsDependent()... http://lxr.mozilla.org/seamonkey/source/xpcom/reflect/xptinfo/public/xptinfo.h#140 I just don't buy this at all. I realize that this was probably just (as much as anyting) an experiment. I can understand your possible dismay at the complex top-down method that has evolved here. But, rewrites have cost and cost requires payoff. I'm not seeing that here.
Attachment #60489 - Flags: needs-work+
Yes, it was weekend experiment. I posted it for future reference as something to think about if this code ever went through a major overhaul. I didn't mean for anyone to waste time on it, I should have made that intention explicit in the comment.
Retargetting to the proper post 1.0 milestone
Target Milestone: mozilla1.0.1 → mozilla1.2
Moving out to 1.3. If this needs to be in before 1.3 please comment.
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
Moving to 1.4 Alpha
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
Moving out
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
Target Milestone: mozilla1.5alpha → mozilla1.5beta
Moving out, speak up if you believe this needs to be considered for 1.5b
Target Milestone: mozilla1.5beta → mozilla1.6alpha
Moving out
Target Milestone: mozilla1.6alpha → mozilla1.7alpha
QA Contact: pschwartau → xpconnect
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: