Martin Mucha has posted comments on this change. Change subject: core: MacPoolManager strategy, alternative implementation, test for benchmarks ......................................................................
Patch Set 18: (62 comments) answers. http://gerrit.ovirt.org/#/c/26405/18//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-04-03 15:34:31 +0200 Line 4: Commit: Martin Mucha <mmu...@redhat.com> Line 5: CommitDate: 2014-05-16 08:40:03 +0200 Line 6: Line 7: core: MacPoolManager strategy, alternative implementation, test for > Pretty sure this needs to be updated. Done Line 8: benchmarks Line 9: Line 10: * internals of MacPoolManager moved to separate implementation of Line 11: MacPoolManagerStrategy (MacPoolManagerStrategyOriginal) to allow Line 31: *MacPoolManager is singleton; only 'getInstance()' has to be Line 32: static other methods should be instance methods. Having them Line 33: static is mixing concepts. Line 34: --- Line 35: space costs(for 1 000 000 MACs range): > All the information bought forth is nice and very informative, but it would ok, can you point me to proper wiki page? This should probably not go to feature page. should this be removed from this message? It documents what have been done and with what outcome ... Line 36: (these measurements need not be accurate, but it's probably the Line 37: best you can get from JVM): Line 38: Measurement for: MacPoolManagerOriginal------ Line 39: Time: 794 ms http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/pom.xml File backend/manager/modules/bll/pom.xml: Line 18: <engine.schema>engine</engine.schema> Line 19: </properties> Line 20: Line 21: <dependencies> Line 22: > Not necessary to touch pom.xml at all, right? Done Line 23: <dependency> Line 24: <groupId>${engine.groupId}</groupId> Line 25: <artifactId>compat</artifactId> Line 26: <version>${engine.version}</version> http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerOriginal.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerOriginal.java: Line 21 Line 22 Line 23 Line 24 Line 25 > Due to this change, org.ovirt.engine.core.utils.MacAddressRangeUtils.initRa I remove unused method; that's not a problem even in this patch. http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerRanges.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerRanges.java: Line 19: public class MacPoolManagerRanges implements MacPoolManagerStrategy{ Line 20: Line 21: private static final Log log = LogFactory.getLog(MacPoolManagerRanges.class); Line 22: Line 23: private final int maxMacsInPool; > This should be deleted since there will be no possibility of memory leak, a I don't know intentions behind each config value. This is still valid config value. Until we deprecate it, it should remain functional. I'm not aware of request of deprecating this value and if there is such one(let me know) it should be done in another commit. Line 24: private final String rangesString; Line 25: Line 26: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 27: private final Boolean allowDuplicates; Line 23: private final int maxMacsInPool; Line 24: private final String rangesString; Line 25: Line 26: private final ReentrantReadWriteLock lockObj = new ReentrantReadWriteLock(); Line 27: private final Boolean allowDuplicates; > Would null value make sense? Done Line 28: private boolean initialized; Line 29: private MacsStorage macsStorage; Line 30: Line 31: public MacPoolManagerRanges(Integer maxMacsInPool, String rangesString, Boolean allowDuplicates) { Line 35: } Line 36: Line 37: @Override Line 38: public void initialize() { Line 39: checkNotInitialized(); > This isn't thread safe. Done Line 40: Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 40: Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: > Not sure why there are so many empty lines between some code, and no new li Done Line 45: this.macsStorage = createMacsStorage(rangesString); Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 41: lockObj.writeLock().lock(); Line 42: try { Line 43: log.infoFormat("Start initializing "+getClass().getSimpleName()); Line 44: Line 45: this.macsStorage = createMacsStorage(rangesString); > Don't call with this Done Line 46: Line 47: Line 48: List<VmNic> interfaces = getVmNicInterfacesFromDB(); Line 49: Line 50: for (VmNic iface : interfaces) { Line 51: forceAddMacWithoutLocking(iface.getMacAddress()); Line 52: } Line 53: initialized = true; Line 54: log.infoFormat("Finished initializing. Available MACs in pool: {0}", this.macsStorage.getAvailableMacsCount()); > Don't call with this Done Line 55: } catch (Exception ex) { Line 56: log.errorFormat("Error in initializing MAC Addresses pool manager.", ex); Line 57: } finally { Line 58: lockObj.writeLock().unlock(); Line 61: Line 62: private MacsStorage createMacsStorage(String rangesString) { Line 63: List<Pair<Long, Long>> rangesBoundaries = MacAddressRangeUtils.rangesTextualDefinitionToRangeBoundaries(rangesString); Line 64: int macsToCreate = maxMacsInPool; Line 65: MacsStorage macsStorage = new MacsStorage(this.allowDuplicates); > Don't call with this Done Line 66: for (Pair<Long, Long> rangesBoundary : rangesBoundaries) { Line 67: macsToCreate -= macsStorage.addRange(rangesBoundary.getFirst(), rangesBoundary.getSecond(), macsToCreate); Line 68: } Line 69: Line 62: private MacsStorage createMacsStorage(String rangesString) { Line 63: List<Pair<Long, Long>> rangesBoundaries = MacAddressRangeUtils.rangesTextualDefinitionToRangeBoundaries(rangesString); Line 64: int macsToCreate = maxMacsInPool; Line 65: MacsStorage macsStorage = new MacsStorage(this.allowDuplicates); Line 66: for (Pair<Long, Long> rangesBoundary : rangesBoundaries) { > Shouldn't the single instance be called just range? rewritten to use LongRange and uses name range. Line 67: macsToCreate -= macsStorage.addRange(rangesBoundary.getFirst(), rangesBoundary.getSecond(), macsToCreate); Line 68: } Line 69: Line 70: if (macsStorage.noAvailableMacs()) { Line 68: } Line 69: Line 70: if (macsStorage.noAvailableMacs()) { Line 71: throw new VdcBLLException(VdcBllErrors.MAC_POOL_INITIALIZATION_FAILED); Line 72: } else { > No need for else clause here we've already discussed it many times. Semantically full if-else construct is used for condition checking, only if with return is used for invariants. This is not an invariant. Why do you insist on code compression? This is normal java code construct. Line 73: return macsStorage; Line 74: } Line 75: } Line 76: Line 73: return macsStorage; Line 74: } Line 75: } Line 76: Line 77: private List<VmNic> getVmNicInterfacesFromDB() { > s/DB/Db/ Done Line 78: return DbFacade.getInstance().getVmNicDao().getAll(); Line 79: } Line 80: Line 81: private void logMacPoolEmpty() { Line 77: private List<VmNic> getVmNicInterfacesFromDB() { Line 78: return DbFacade.getInstance().getVmNicDao().getAll(); Line 79: } Line 80: Line 81: private void logMacPoolEmpty() { > I would put the "if (macsStorage.noAvailableMacs()) {" inside, since it's p nice observation. DONE. Line 82: AuditLogableBase logable = new AuditLogableBase(); Line 83: AuditLogDirector.log(logable, AuditLogType.MAC_POOL_EMPTY); Line 84: } Line 85: Line 84: } Line 85: Line 86: @Override Line 87: public String allocateNewMac() { Line 88: checkInitialized(); > This isn't thread safe Done Line 89: lockObj.writeLock().lock(); Line 90: try { Line 91: return allocateNewMacsWithoutLocking(1).get(0); Line 92: } finally { Line 94: } Line 95: } Line 96: Line 97: private List<String> allocateNewMacsWithoutLocking(int numberOfMacs) { Line 98: if (!initialized) { > Why do you need this if you already checked if initialized in both places c Done Line 99: logInitializationError("Failed to allocate new Mac address."); Line 100: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 101: } Line 102: Line 110: } Line 111: Line 112: @Override Line 113: public int getAvailableMacsCount() { Line 114: checkInitialized(); > This isn't thread safe Done Line 115: lockObj.readLock().lock(); Line 116: try { Line 117: if (!initialized) { Line 118: logInitializationError("Failed to get available Macs count."); Line 113: public int getAvailableMacsCount() { Line 114: checkInitialized(); Line 115: lockObj.readLock().lock(); Line 116: try { Line 117: if (!initialized) { > Why check again if initialized? Done Line 118: logInitializationError("Failed to get available Macs count."); Line 119: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NOT_INITIALIZED); Line 120: } Line 121: Line 128: } Line 129: Line 130: @Override Line 131: public void freeMac(String mac) { Line 132: checkInitialized(); > This isn't thread safe Done Line 133: lockObj.writeLock().lock(); Line 134: try { Line 135: if (!initialized) { Line 136: logInitializationError("Failed to free mac address " + mac + " ."); Line 131: public void freeMac(String mac) { Line 132: checkInitialized(); Line 133: lockObj.writeLock().lock(); Line 134: try { Line 135: if (!initialized) { > Why check again if initialized? Done Line 136: logInitializationError("Failed to free mac address " + mac + " ."); Line 137: } else { Line 138: macsStorage.freeMac(MacAddressRangeUtils.macToLong(mac)); Line 139: } Line 150: } Line 151: Line 152: @Override Line 153: public boolean addMac(String mac) { Line 154: checkInitialized(); > This isn't thread safe Done Line 155: lockObj.writeLock().lock(); Line 156: try { Line 157: boolean added = this.macsStorage.useMac(MacAddressRangeUtils.macToLong(mac)); Line 158: if (macsStorage.noAvailableMacs()) { Line 153: public boolean addMac(String mac) { Line 154: checkInitialized(); Line 155: lockObj.writeLock().lock(); Line 156: try { Line 157: boolean added = this.macsStorage.useMac(MacAddressRangeUtils.macToLong(mac)); > Don't call with this Done Line 158: if (macsStorage.noAvailableMacs()) { Line 159: logMacPoolEmpty(); Line 160: } Line 161: return added; Line 167: } Line 168: Line 169: @Override Line 170: public void forceAddMac(String mac) { Line 171: checkInitialized(); > This isn't thread safe Done Line 172: lockObj.writeLock().lock(); Line 173: try { Line 174: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macToLong(mac)); Line 175: if (macsStorage.noAvailableMacs()) { Line 170: public void forceAddMac(String mac) { Line 171: checkInitialized(); Line 172: lockObj.writeLock().lock(); Line 173: try { Line 174: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macToLong(mac)); > Why not call forceAddMacWithoutLocking? Done Line 175: if (macsStorage.noAvailableMacs()) { Line 176: logMacPoolEmpty(); Line 177: } Line 178: } finally { Line 180: } Line 181: } Line 182: Line 183: private void forceAddMacWithoutLocking(String mac) { Line 184: this.macsStorage.useMacNoDuplicityCheck(MacAddressRangeUtils.macToLong(mac)); > Don't call with this Done Line 185: if (macsStorage.noAvailableMacs()) { Line 186: logMacPoolEmpty(); Line 187: } Line 188: } Line 188: } Line 189: Line 190: @Override Line 191: public boolean isMacInUse(String mac) { Line 192: checkInitialized(); > This isn't thread safe Done Line 193: lockObj.readLock().lock(); Line 194: try { Line 195: return this.macsStorage.isMacInUse(MacAddressRangeUtils.macToLong(mac)); Line 196: Line 191: public boolean isMacInUse(String mac) { Line 192: checkInitialized(); Line 193: lockObj.readLock().lock(); Line 194: try { Line 195: return this.macsStorage.isMacInUse(MacAddressRangeUtils.macToLong(mac)); > Don't call with this Done Line 196: Line 197: } finally { Line 198: lockObj.readLock().unlock(); Line 199: } Line 199: } Line 200: } Line 201: Line 202: @Override Line 203: public void freeMacs(List<String> macs) { //TODO MM: how about some duplicities here? Release it twice or what? > What did original code do? released duplicate macs number of times. Preserving behavior, todo removed. Line 204: checkInitialized(); Line 205: if (!macs.isEmpty()) { Line 206: lockObj.writeLock().lock(); Line 207: try { Line 200: } Line 201: Line 202: @Override Line 203: public void freeMacs(List<String> macs) { //TODO MM: how about some duplicities here? Release it twice or what? Line 204: checkInitialized(); > This isn't thread safe Done Line 205: if (!macs.isEmpty()) { Line 206: lockObj.writeLock().lock(); Line 207: try { Line 208: for (String mac : macs) { Line 216: } Line 217: Line 218: @Override Line 219: public List<String> allocateMacAddresses(int numberOfAddresses) { Line 220: checkInitialized(); > This isn't thread safe Done Line 221: lockObj.writeLock().lock(); Line 222: try { Line 223: List<String> macAddresses = this.allocateNewMacsWithoutLocking(numberOfAddresses); Line 224: return macAddresses; Line 219: public List<String> allocateMacAddresses(int numberOfAddresses) { Line 220: checkInitialized(); Line 221: lockObj.writeLock().lock(); Line 222: try { Line 223: List<String> macAddresses = this.allocateNewMacsWithoutLocking(numberOfAddresses); > Don't call with this Done Line 224: return macAddresses; Line 225: } finally { Line 226: lockObj.writeLock().unlock(); Line 227: } Line 220: checkInitialized(); Line 221: lockObj.writeLock().lock(); Line 222: try { Line 223: List<String> macAddresses = this.allocateNewMacsWithoutLocking(numberOfAddresses); Line 224: return macAddresses; > You can return immediately, no need for variable Done Line 225: } finally { Line 226: lockObj.writeLock().unlock(); Line 227: } Line 228: } Line 233: throw new IllegalStateException("This instance was already initialized."); Line 234: } Line 235: } Line 236: Line 237: private void checkInitialized() { > You should call logInitializationError and also throw a VdcBLLException(Vdc I hope I did not overlook something but I think it's not called where it formerly wasn't. Line 238: if (!initialized) { Line 239: throw new IllegalStateException("This instance not yet initialized."); Line 240: } Line 241: Line 241: Line 242: } Line 243: Line 244: @Override Line 245: public void copyContent(MacPoolManagerStrategy macPoolManagerStrategy) { > This method is unused.. Done Line 246: macsStorage.copyAllocatedCustomMacs(macPoolManagerStrategy); Line 247: macsStorage.copyAllocatedMacs(macPoolManagerStrategy); Line 248: } Line 249: http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerStrategy.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerStrategy.java: Line 59: * @throws org.ovirt.engine.core.common.errors.VdcBLLException if mac address cannot be allocated. Line 60: */ Line 61: List<String> allocateMacAddresses(int numberOfAddresses); Line 62: Line 63: void copyContent(MacPoolManagerStrategy macPoolManagerStrategy); > Why add unused method? 1. I've moved method to another patchset 2. I've renamed it to copyContentInto — I cannot satisfy your requested parameter to act as source, since there are no public method for iterating over MACs and user-macs; in fact public api does not reveal there's is such distinction among macs. I do believe this is better, but this comes at price parameter being target, not source. Line 64: Line 65: http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacsStorage.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacsStorage.java: Line 28: } Line 29: } Line 30: Line 31: long size = i - rangeStart; Line 32: if (size > Integer.MAX_VALUE) { > If you're already limiting to Integer.MAX_VALUE, there is no sense to look you're right it can be done this way, but this is refactor — i just left what was formerly there. Looping for multicasts removal is fixed in following changeset by fixing input ranges. I think this should be ok. Line 33: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Line 34: } Line 35: Line 36: if (numberOfNonMultiCasts > 0) { Line 29: } Line 30: Line 31: long size = i - rangeStart; Line 32: if (size > Integer.MAX_VALUE) { Line 33: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); > Since you decided to limit this, please take care of upgrade so that if the I did not decide to introduce this "limitation". I decided to check this. This was always a "limitation". You cannot stick more entries into java collections. Next. Engine hw requirements are 16GB of RAM (fow webapp, wow). Just to use system to this 'limitation' you would need 2^32 of 64b pointers (hashset is backed by hashmap, so take it twice) + 2^32 of 17B representation of MAC. If I'm not mistaken this sums up to 142GB of RAM minimum just for pool. Is there any chance that there exist customer with such deployment? Never heard a java program using 30GB of ram, although there exist JVM claiming they can handle 0.5TB or RAM. My point: if somebody had used such setting, app was not able to start or run. Now he got better exception than OutOfMemory. I can revert that if needed, ok, but it's pointless, from mine point of view. NOBODY could have use that settings. Line 34: } Line 35: Line 36: if (numberOfNonMultiCasts > 0) { Line 37: //java. arrays/collections can not be bigger than 2^32. Line 33: throw new IllegalArgumentException("Not supported that wide ranges(" + size + ")."); Line 34: } Line 35: Line 36: if (numberOfNonMultiCasts > 0) { Line 37: //java. arrays/collections can not be bigger than 2^32. > Not sure why this comment is relevant it relates to casting long to int. User supplies range can contain more MACs than fits to integer. For that reason there's check, cast and explanation in comment. Line 38: ranges.add(new Range(rangeStart, (int)size, numberOfNonMultiCasts)); Line 39: } Line 40: return numberOfNonMultiCasts; Line 41: } Line 43: public boolean useMac(long mac) { Line 44: return useMac(mac, allowDuplicates); Line 45: } Line 46: Line 47: private boolean useMac(long mac, Boolean allowDuplicates) { > What is the significance of a null allowDuplicates? Done Line 48: testForMultiCastMac(mac); Line 49: Line 50: Range range = findIncludingRange(mac); Line 51: if (range == null) { Line 48: testForMultiCastMac(mac); Line 49: Line 50: Range range = findIncludingRange(mac); Line 51: if (range == null) { Line 52: return this.customMacs.add(mac); > Don't use this Done Line 53: } else { Line 54: return range.use(mac, allowDuplicates); Line 55: } Line 56: } Line 62: public boolean isMacInUse(long mac) { Line 63: testForMultiCastMac(mac); Line 64: Line 65: Range range = findIncludingRange(mac); Line 66: return range == null ? this.customMacs.contains(mac) : range.isAllocated(mac); > Don't use this Done Line 67: } Line 68: Line 69: public void freeMac(long mac) { Line 70: testForMultiCastMac(mac); Line 70: testForMultiCastMac(mac); Line 71: Line 72: Range range = findIncludingRange(mac); Line 73: if (range == null) { Line 74: this.customMacs.remove(mac); > Don't use this Done Line 75: } else { Line 76: range.freeMac(mac); Line 77: } Line 78: } Line 76: range.freeMac(mac); Line 77: } Line 78: } Line 79: Line 80: public boolean noAvailableMacs() { > Please change to positive method, as double negative is harder to maintain Done Line 81: for (Range range : ranges) { Line 82: if (range.getAvailableCount() > 0) { Line 83: return false; Line 84: } Line 85: } Line 86: return true; Line 87: } Line 88: Line 89: public List<Long> allocateAvailableMac(int numberOfMacs) { > Should be allocateAvailableMacs Done Line 90: if (getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? Line 91: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 92: } Line 93: Line 86: return true; Line 87: } Line 88: Line 89: public List<Long> allocateAvailableMac(int numberOfMacs) { Line 90: if (getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? > Don't allocate any MACs if there aren't enough Done Line 91: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 92: } Line 93: Line 94: return getRangeWithAvailableMac().allocateMac(numberOfMacs); Line 90: if (getAvailableMacsCount() < numberOfMacs) { //TODO MM: allocate just those remaining or none? Line 91: throw new VdcBLLException(VdcBllErrors.MAC_POOL_NO_MACS_LEFT); Line 92: } Line 93: Line 94: return getRangeWithAvailableMac().allocateMac(numberOfMacs); > What if there is no one range with the free amount of MACs, but across seve Done Line 95: } Line 96: Line 97: private Range getRangeWithAvailableMac() { Line 98: for (Range range : ranges) { Line 110: } Line 111: return count; Line 112: } Line 113: Line 114: private void testForMultiCastMac(long mac) { > No need for this method I don't get why. It block user from supplying multicast address. Which should be blocked, no? Line 115: if (MacAddressRangeUtils.macIsMulticast(mac)) { Line 116: throw new IllegalArgumentException("You're trying to work with multicast address."); //TODO MM: should this be prohibited? Line 117: } Line 118: } Line 125: } Line 126: return null; Line 127: } Line 128: Line 129: public void copyAllocatedCustomMacs(MacPoolManagerStrategy macPoolManagerStrategy) { > Please remove and add when used Done Line 130: for (Long mac : customMacs) { Line 131: final int count = customMacs.count(mac); Line 132: final String macString = MacAddressRangeUtils.macToString(mac); Line 133: Line 136: } Line 137: } Line 138: } Line 139: Line 140: public void copyAllocatedMacs(MacPoolManagerStrategy macPoolManagerStrategy) { > Please remove and add when used Done Line 141: for (Range range : ranges) { Line 142: range.copyAllocatedMacs(macPoolManagerStrategy); Line 143: } Line 144: } http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/Range.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/Range.java: Line 12: private int notUsedCount; Line 13: Line 14: private UsedMacs usedMacs; Line 15: Line 16: public boolean contains(long mac) { > Why is this before the c'tor? Done Line 17: return offset <= mac && (offset + size) >= mac; Line 18: } Line 19: Line 20: public Range(long offset, int size, int numberOfNonMultiCasts) { Line 21: this.offset = offset; Line 22: this.size = size; Line 23: this.notUsedCount = numberOfNonMultiCasts; Line 24: Line 25: macUsageCount = new int[size]; > This seems very problematic to me, if user defines a range of same OUI (i.e yes, you're right. I don't have idea how to make it any better and keep the code clean. If you have idea, let me know. Comment: as I got it Collections are out of game (wrappers, pointers) and so are common data structures since 64b pointers are really expensive (even 32b pointers are when used with CompressedOops). Unless we know threshold for maximum MAC duplicate usage or unless we block duplicate usage, I don't know how to improve that. Line 26: this.usedMacs = new UsedMacsBitSet(size); Line 27: } Line 28: Line 29: /** Line 29: /** Line 30: * Line 31: * @param mac mac to add Line 32: * @return if mac was used (it's usage count was increased). I.e. if it was not used, it's used now, or Line 33: * it was used and duplicates are allowed so it's not used some more. > "so it's not used some more"? Done Line 34: */ Line 35: public boolean use(long mac, boolean allowDuplicates) { Line 36: try { Line 37: int index = macToArrayIndex(mac); Line 34: */ Line 35: public boolean use(long mac, boolean allowDuplicates) { Line 36: try { Line 37: int index = macToArrayIndex(mac); Line 38: int currentCount = currentCount(index); > Why assign to variable that is used only once? Done Line 39: Line 40: if (currentCount == 0) { Line 41: notUsedCount--; Line 42: this.usedMacs.removeMacFromNotUsed(macToArrayIndex(mac)); Line 49: return true; Line 50: } Line 51: Line 52: return false; Line 53: } catch (Exception e) { > What exception do you expect to happen? can't remember why I put that in. Removed. Line 54: throw new RuntimeException("failed use during adding mac: " + mac + ", index = " + (mac - offset), e); Line 55: } Line 56: Line 57: } Line 55: } Line 56: Line 57: } Line 58: Line 59: private int currentCount(int index) { > what is the necessity of this method? Done Line 60: return macUsageCount[index]; Line 61: } Line 62: Line 63: private int macToArrayIndex(long mac) { Line 65: } Line 66: Line 67: public boolean isAllocated(long mac) { Line 68: int index = macToArrayIndex(mac); Line 69: int currentCount = macUsageCount[index]; > This can all be inlined don't you think it's more readable this way? Line 70: return currentCount > 0; Line 71: } Line 72: Line 73: public void freeMac(long mac) { Line 70: return currentCount > 0; Line 71: } Line 72: Line 73: public void freeMac(long mac) { Line 74: int index = macToArrayIndex(mac); > This can be inlined don't you think it's more readable this way? Line 75: int currentCount = macUsageCount[index]; Line 76: if (currentCount == 0) { Line 77: return; Line 78: } Line 89: public int getAvailableCount() { Line 90: return notUsedCount; Line 91: } Line 92: Line 93: public List<Long> allocateMac(int numberOfMacs) { > Should be called allocateMacs who told you that? what's wrong with 'result'? 'Result' of function computation ... Why that seems odd? (Actually this is recommended way in many languagues; http://www.javapractices.com/topic/TopicAction.do?Id=63) Line 94: List<Long> result = new ArrayList<>(numberOfMacs); Line 95: Line 96: int count = 0; Line 97: while (count < numberOfMacs) { Line 93: public List<Long> allocateMac(int numberOfMacs) { Line 94: List<Long> result = new ArrayList<>(numberOfMacs); Line 95: Line 96: int count = 0; Line 97: while (count < numberOfMacs) { > Why not for loop? Done Line 98: final long mac = usedMacs.unusedMac(offset); Line 99: use(mac, false); //well duplicates may be allowed, but we're using unallocated mac. Line 100: count++; Line 101: result.add(mac); Line 95: Line 96: int count = 0; Line 97: while (count < numberOfMacs) { Line 98: final long mac = usedMacs.unusedMac(offset); Line 99: use(mac, false); //well duplicates may be allowed, but we're using unallocated mac. > Please write comment before code. Done Line 100: count++; Line 101: result.add(mac); Line 102: } Line 103: http://gerrit.ovirt.org/#/c/26405/18/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/UsedMacsBitSet.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/UsedMacsBitSet.java: Line 3: import java.util.BitSet; Line 4: Line 5: import org.ovirt.engine.core.utils.MacAddressRangeUtils; Line 6: Line 7: class UsedMacsBitSet implements UsedMacs { > This class is unnecessary and confusing, please remove it why is unnecessary? If I'm not mistaken, then this class is used to search unused mac, because linear search for MAC with usage count == 0 is very slow in large ranges. (introduced after actual measuring pool performance — after lots of fixes the measuring is of course gone, but still. This improved overall performance a lot). Line 8: Line 9: private BitSet bitSet; Line 10: Line 11: public UsedMacsBitSet(int size) { -- To view, visit http://gerrit.ovirt.org/26405 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I69a5c1b3b43966e49fa6039597c06966ce514618 Gerrit-PatchSet: 18 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
