Hi All Please see my *changed* comments below.
>> > Mine is against using "ThreadLocalRandomSource"... >> -- What is the wayout other than that. Please suggest. >I think I did. >>*--* The factory based approach would be useful only when we can have separate copies of operators for each set of operations. *--* *T*he factory based approach can introduce *custom* RNG, but it can improve performance only when we can have separate copies of operators for each set of operations which might lead to *memory issues* as explained in previous mail. Thanks & Regards --Avijit Basak On Wed, 22 Dec 2021 at 18:54, Avijit Basak <avijit.ba...@gmail.com> wrote: > Hi All > > Please see my comments below. > > >> >Several problems with this approach (raised in previous messages IIRC): > >> >1. Potential performance loss in sharing the same RNG instance. > >> -- As per my understanding ThreadLocalRandomSource creates separate > >> instances of UniformRandomProvider for each thread. So I am not sure > how a > >> UniformRandomProvider instance is being shared. Please correct me if I > am > >> wrong. > > >Within a given thread there will be *one* RNG instance; that's what I > meant > >by "shared". > >Of course you are right that that instance is not shared by multiple > threads > >(which would be a bug). > >The performance loss is because it will be necessary to call > > ThreadLocalRandomSource.current(RandomSource source) > >for each access to the RNG (since it would be a bug to store the returned > >value in e.g. an operator instance that would be shared among threads (as > >you suggest below). > > -- I tried to do a small test on it and here are the results. Output times > are in milliseconds. According to my understanding the performance loss is > mostly during creation of per thread instance of UniformRandomProvider. > --*CUT*-- > @Test > void test() { > int limit = 1; > long start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 1000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 10000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 100000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 1000000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 10000000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 100000000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > > limit = 1000000000; > start = System.currentTimeMillis(); > for (int i = 0; i < limit; i++) { > ThreadLocalRandomSource.current(RandomSource.JDK); > } > System.out.println(System.currentTimeMillis() - start); > } > --*CUT*-- > --*output*-- > 363 > 1 > 2 > 4 > 6 > 28 > 244 > 2423 > --*output*-- > > >> >2. Less/no flexibility (no user's choice of random source). > >> -- Agreed. > -- Do we really need this much flexibility here? > >> >3. Error-prone (user can access/reuse the "UniformRandomProvider" > >> instances). > >> > >> >Again: "ThreadLocalRandomSource" is an ad-hoc workaround for correct > but > >> >"light" usage of random number generation in a multi-threaded > application; > >> GAs > >> >make "heavy" use of RNG, thus it is does not seem outlandish that all > the > >> RNG > >> >"clients" (e.g. every "operator") creates their own instances. > > > > > >> >IMHO, a more important discussion would be about the expectations in a > >> >multithreaded context: E.g. should an operator be shareable by > different > >> >threads? And if not, how does the API help application developers to > avoid > >> >such pitfalls? > >> -- Once we implement multi-threading in GA, same crossover and mutation > >> operators will be re-used across multiple threads. > > >I would be wary to go on that path; better consider making (deep) copies. > >We can have multiple instances of an operator, all being configured in the > >same way but being different instances with no risk of a multithreading > bug. > > -- I don't think this would be a good design choice just to support > customization of RNG functionality. This will lead to too many instances of > the same operators resulting in lots of unnecessary memory consumption. I > think we might face memory issues for higher dimensional problems. As > population size requirement also increases with increase of dimension this > might lead to a major issue and need a thought. > So I think we have a design tradeoff here performance vs memory > consumption. I am more worried about memory as that might restrict use of > this library beyond a certain number of dimensions in some areas. However, > creating deep copy would only be possible when we strictly restrict > extension of operators which I want to avoid. > > >> So even if we provide > >> the customization at the operator level we cannot avoid sharing. > > >We can, and we should. > >What we probably can't avoid sharing is the instance that represents the > >population of chromosomes. > *--* In a multi-threaded optimization the chromosome instances are shared > in case the same chromosome is chosen for crossover by the selection > process. I missed this point earlier. > ... > > >> > Mine is against using "ThreadLocalRandomSource"... > >> -- What is the wayout other than that. Please suggest. > > >I think I did. > *--* The factory based approach would be useful only when we can have > separate copies of operators for each set of operations. > > >Maybe it's time to create a dedicated branch for the GA functionality > >so that we can try out the different approaches. > > > > > > >> I think first we need to decide on whether we really need this > > >> customization and if yes then why. Then we can decide on alternate > > >> implementation options. > > > > > >> >As per the recent updates of the math-related code bases, the > > >> >public API should provide factory methods (constructors should > > >> >be private). > > >> -- private constructors will make public API classes non-extensible. > This > > >> will severely restrict the extensibility of this framework which I > want > > to > > >> avoid. I am not sure why we need to remove public constructors. It > would > > be > > >> helpful if you could refer me to any relevant discussion thread. > > > > > Allowing extensibility is a huge burden on library maintainers. The > > > library must have been designed to support it; hence, you should > > > first describe what kind(s) of extensions (with usage examples) you > > > have in mind. > > --The library should be extensible to support customization. Users should > > be able to customise or provide their own implementation of genetic > > operators for crossover and mutation. The chromosome classes should also > be > > open for extension. > > >I don't get why we should support extensions outside this library. > *--* I think we should not block the extension. > > >Initially we discussed about having a light-weight library, for easier > usage > >than alternative existing framework(s). > *--* We can always think of making the framework lightweight but it > should not cost extensibility. > > >> E.g. any developer should be able to extend the > >> IntegralChromosome class and define a child class which explicitly > >> specifies the range of integers to be used. > > >It does not look like this would need an extension, only configuration > >of the range. > *-- *I agree. But the question is should we block the extension. > > >> I have initially implemented > >> the Binary chromosome and the corresponding binary mutation following > the > >> same pattern. However, restricting extension of concrete classes by > private > >> constructor does not prevent users from extending the abstract parent > >> classes. > > >We should aim at coding the GA logic through (Java) interfaces, and not > >expose the "abstract" classes. > *-- *One of the primary reasons for me to contribute in Apache' GA > library is it's simplicity and extensibility. I would like to have a > framework which should be always extensible for any problem domain with > minor changes. The primary reason behind this is that application domains > of GA are too diverse. It is not possible to implement everything in a > library. We don't know all possible domain areas too. If we remove the > extensibility from the framework it would be useless in lots of areas. > > >Extending the functionality, if necessary, should be contributed back here > *-- *Sometimes the GA operators are very much specific to the domain and > it's hard to generalise. In those scenarios contributing back to the > library might not be possible. However, if a library cannot be extended for > a new domain by users it becomes underutilised over time if not useless. > > > Thanks & Regards > --Avijit Basak > > On Tue, 21 Dec 2021 at 22:05, Gilles Sadowski <gillese...@gmail.com> > wrote: > >> Hello. >> >> Le mar. 21 déc. 2021 à 16:21, Avijit Basak <avijit.ba...@gmail.com> a >> écrit : >> > >> > Hi All >> > >> > Please see my comments. Sorry for the delayed response. >> > >> > >Several problems with this approach (raised in previous messages IIRC): >> > >1. Potential performance loss in sharing the same RNG instance. >> > -- As per my understanding ThreadLocalRandomSource creates separate >> > instances of UniformRandomProvider for each thread. So I am not sure >> how a >> > UniformRandomProvider instance is being shared. Please correct me if I >> am >> > wrong. >> >> Within a given thread there will be *one* RNG instance; that's what I >> meant >> by "shared". >> Of course you are right that that instance is not shared by multiple >> threads >> (which would be a bug). >> The performance loss is because it will be necessary to call >> ThreadLocalRandomSource.current(RandomSource source) >> for each access to the RNG (since it would be a bug to store the returned >> value in e.g. an operator instance that would be shared among threads (as >> you suggest below). >> >> > >2. Less/no flexibility (no user's choice of random source). >> > -- Agreed. >> > >3. Error-prone (user can access/reuse the "UniformRandomProvider" >> > instances). >> > >> > >Again: "ThreadLocalRandomSource" is an ad-hoc workaround for correct >> but >> > >"light" usage of random number generation in a multi-threaded >> application; >> > GAs >> > >make "heavy" use of RNG, thus it is does not seem outlandish that all >> the >> > RNG >> > >"clients" (e.g. every "operator") creates their own instances. >> > >> > >> > >IMHO, a more important discussion would be about the expectations in a >> > >multithreaded context: E.g. should an operator be shareable by >> different >> > >threads? And if not, how does the API help application developers to >> avoid >> > >such pitfalls? >> > -- Once we implement multi-threading in GA, same crossover and mutation >> > operators will be re-used across multiple threads. >> >> I would be wary to go on that path; better consider making (deep) copies. >> We can have multiple instances of an operator, all being configured in the >> same way but being different instances with no risk of a multithreading >> bug. >> >> > So even if we provide >> > the customization at the operator level we cannot avoid sharing. >> >> We can, and we should. >> What we probably can't avoid sharing is the instance that represents the >> population of chromosomes. >> >> > >> > >> My original implementation did not allow any customization of >> > RandomSource >> > >> instances. There was a thought in review for customization of >> > RandomSource, >> > >> so these options were considered. I don't think this would make any >> > >> difference to algorithm functionality. >> > >> > > Quite right. But the customization can come at zero cost for the >> users >> > > who don't need it. Admittedly it's a little more work on the part of >> the >> > > developer(s) but it's a one off cost (and I'm fine working on that >> part >> > of >> > > the library once other, more important, things have been settled). >> > >> > >> Even earlier I used Math.random() >> > >> which worked equally well. So my *vote* should be *against* this >> > >> customization. >> > >> > > Mine is against using "ThreadLocalRandomSource"... >> > -- What is the wayout other than that. Please suggest. >> >> I think I did. >> Maybe it's time to create a dedicated branch for the GA functionality >> so that we can try out the different approaches. >> >> > >> > >> I think first we need to decide on whether we really need this >> > >> customization and if yes then why. Then we can decide on alternate >> > >> implementation options. >> > > >> > >> >As per the recent updates of the math-related code bases, the >> > >> >public API should provide factory methods (constructors should >> > >> >be private). >> > >> -- private constructors will make public API classes non-extensible. >> This >> > >> will severely restrict the extensibility of this framework which I >> want >> > to >> > >> avoid. I am not sure why we need to remove public constructors. It >> would >> > be >> > >> helpful if you could refer me to any relevant discussion thread. >> > >> > > Allowing extensibility is a huge burden on library maintainers. The >> > > library must have been designed to support it; hence, you should >> > > first describe what kind(s) of extensions (with usage examples) you >> > > have in mind. >> > --The library should be extensible to support customization. Users >> should >> > be able to customise or provide their own implementation of genetic >> > operators for crossover and mutation. The chromosome classes should >> also be >> > open for extension. >> >> I don't get why we should support extensions outside this library. >> Initially we discussed about having a light-weight library, for easier >> usage >> than alternative existing framework(s). >> >> > E.g. any developer should be able to extend the >> > IntegralChromosome class and define a child class which explicitly >> > specifies the range of integers to be used. >> >> It does not look like this would need an extension, only configuration >> of the range. >> >> > I have initially implemented >> > the Binary chromosome and the corresponding binary mutation following >> the >> > same pattern. However, restricting extension of concrete classes by >> private >> > constructor does not prevent users from extending the abstract parent >> > classes. >> >> We should aim at coding the GA logic through (Java) interfaces, and not >> expose the "abstract" classes. >> Extending the functionality, if necessary, should be contributed back >> here. >> >> Regards, >> Gilles >> >> >>> [...] >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > > -- > Avijit Basak > -- Avijit Basak