Opened 2 years ago

Closed 22 months ago

#2653 closed defect (done)

GetEstimatedValues is not thread-safe for k-nearest neighbours model

Reported by: gkronber Owned by: gkronber
Priority: medium Milestone: HeuristicLab 3.3.15
Component: Algorithms.DataAnalysis Version: 3.3.14
Keywords: Cc:

Description


Change History (13)

comment:1 Changed 2 years ago by gkronber

  • Owner set to gkronber
  • Status changed from new to accepted

comment:2 Changed 2 years ago by gkronber

r14236: added synchronization in NearestNeighbourModel and made some related changes to GradientBoostedTreesModelSurrogate

comment:3 Changed 2 years ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from accepted to reviewing

comment:4 Changed 22 months ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from reviewing to assigned

Reviewed r14236.

  • I dislike locking on an exposed objects (kdTree) that is modified by some alglib method calls. A private locking object in the NearestNeighbourModel and GBTModelSurrogate would be more appropriate.
  • GBTSurrogateModel should use a double checked locking pattern [0] for handling null values. AFAIK lock on a null reference has no effect.
     lock (actualModel) { if (actualModel == null) actualModel = RecalculateModel(); }
    
    The use of a Lazy<T> for initialization of the actualModel would be suitable and simplify / remove all the scattered locks.

[0] https://en.wikipedia.org/wiki/Double-checked_locking

comment:5 Changed 22 months ago by gkronber

r14314: added a separate lock object for the kdTree in NearestNeighbourModel

comment:6 Changed 22 months ago by gkronber

r14315: using Lazy initialization pattern to remove explicit locking (happens within Lazy instead)

comment:7 Changed 22 months ago by gkronber

  • Owner changed from gkronber to mkommend
  • Status changed from assigned to reviewing

comment:8 Changed 22 months ago by mkommend

r14322: Made lock object readonly in NearestNeighbourModel.

comment:9 follow-up: Changed 22 months ago by mkommend

Btw, I always thought that we use American English for developing, because all the NearestNeighbour classes are written with an U, whereas all other classes containing Neighbor are written differently.

Not a big deal, but annoying to search for.

comment:10 Changed 22 months ago by mkommend

  • Owner changed from mkommend to gkronber
  • Status changed from reviewing to readytorelease

Reviewed r14314 & r14315.

The following changesets must be released: 14236, 14314, 14315 and 14322

comment:11 in reply to: ↑ 9 Changed 22 months ago by gkronber

Michael, thank you for your careful review.

Replying to mkommend:

Btw, I always thought that we use American English for developing, because all the NearestNeighbour classes are written with an U, whereas all other classes containing Neighbor are written differently.

Yes, this is inconsistent, sorry for that. I guess we have to live with some imperfections in HeuristicLab.

comment:12 Changed 22 months ago by gkronber

r14327: merged r14236,r14314:14315 and r14322 from trunk to stable

comment:13 Changed 22 months ago by gkronber

  • Resolution set to done
  • Status changed from readytorelease to closed
Note: See TracTickets for help on using tickets.