Uploaded image for project: 'Blazegraph (by SYSTAP)'
  1. Blazegraph (by SYSTAP)
  2. BLZG-1574 Relayer Journal / RWStore / Transaction interfaces
  3. BLZG-1407

Cherry pick RWStore allocators and related change delta, create PR and merge to master when clean

    Details

    • Type: Sub-task
    • Status: Done
    • Priority: Medium
    • Resolution: Done
    • Affects Version/s: None
    • Fix Version/s: BLAZEGRAPH_RELEASE_1_5_3
    • Component/s: None
    • Labels:
      None

      Issue Links

        Activity

        Hide
        bryanthompson bryanthompson added a comment -

        @martyncutcher has extracted the relevant files into a zip archive.

        @bradbebee Please add zip archive files from @martyncutcher into a branch, push to origin, and create PR for CI feedback.

        Show
        bryanthompson bryanthompson added a comment - @martyncutcher has extracted the relevant files into a zip archive. @bradbebee Please add zip archive files from @martyncutcher into a branch, push to origin, and create PR for CI feedback.
        Hide
        bryanthompson bryanthompson added a comment -

        Brad, per above, please create branch with files, update ticket with branch, and enroll branch into CI with PR.

        Show
        bryanthompson bryanthompson added a comment - Brad, per above, please create branch with files, update ticket with branch, and enroll branch into CI with PR.
        Hide
        beebs Brad Bebee added a comment -

        martyncutcher thompsonbry

        All changes are merged into the rwstore_refactor branch. Please take a look at RWStrategy.java lines 825 and 926. I added a newAllocationContext(boolean isolated) method and commented out a registerContext method.

        Show
        beebs Brad Bebee added a comment - martyncutcher thompsonbry All changes are merged into the rwstore_refactor branch. Please take a look at RWStrategy.java lines 825 and 926. I added a newAllocationContext(boolean isolated) method and commented out a registerContext method.
        Hide
        beebs Brad Bebee added a comment -

        PR is here: https://github.com/SYSTAP/bigdata/pull/130

        May take a bit as we get Jenkins settled into maven builds.

        Show
        beebs Brad Bebee added a comment - PR is here: https://github.com/SYSTAP/bigdata/pull/130 May take a bit as we get Jenkins settled into maven builds.
        Hide
        beebs Brad Bebee added a comment -
        Show
        beebs Brad Bebee added a comment - Merged down in https://github.com/SYSTAP/bigdata/pull/132 .
        Hide
        bryanthompson bryanthompson added a comment -

        Martyn,

        AllocBlock.shadow() has:

        	public void shadow() {
        		// Debug check if commit is different from live
        		for (int i = 0; i < m_commit.length; i++) {
        			if (m_commit[i] != m_live[i]) {
        				System.out.println("live != commit : " + i);
        			}
        		}
        		m_saveCommit = m_commit;
        		m_isoFrees = new int[m_ints]; // ensures we can calculate true differences for reset
        		for (int i = 0; i < m_ints; i++) {
        			m_isoFrees[i] = m_commit[i] & ~m_live[i]; // committed && NOT live
        		}
        		m_commit = m_transients.clone();
        	}
        

        I am commenting out those lines in master. Please review. Let me know if the lines should remain and throw an exception rather than being commented out.

        Thanks,
        Bryan

        Show
        bryanthompson bryanthompson added a comment - Martyn, AllocBlock.shadow() has: public void shadow() { // Debug check if commit is different from live for (int i = 0; i < m_commit.length; i++) { if (m_commit[i] != m_live[i]) { System.out.println("live != commit : " + i); } } m_saveCommit = m_commit; m_isoFrees = new int[m_ints]; // ensures we can calculate true differences for reset for (int i = 0; i < m_ints; i++) { m_isoFrees[i] = m_commit[i] & ~m_live[i]; // committed && NOT live } m_commit = m_transients.clone(); } I am commenting out those lines in master. Please review. Let me know if the lines should remain and throw an exception rather than being commented out. Thanks, Bryan
        Hide
        bryanthompson bryanthompson added a comment -

        Martyn, please review AllocBlock.reset(). The 2nd code path has been modified to call clearCacheBits() rather than directly modifying m_live and m_transients. I just would like confirmation that the code is correct as is.

        				final int chkbits = m_commit[i] & ~m_saveCommit[i]; // new allocations prior to shadow
        				clearCacheBits(cache, startBit, chkbits);
        				
        				// m_live[i] &= ~chkbits;
        				// m_transients[i] &= ~chkbits;
        
        
        Show
        bryanthompson bryanthompson added a comment - Martyn, please review AllocBlock.reset(). The 2nd code path has been modified to call clearCacheBits() rather than directly modifying m_live and m_transients. I just would like confirmation that the code is correct as is. final int chkbits = m_commit[i] & ~m_saveCommit[i]; // new allocations prior to shadow clearCacheBits(cache, startBit, chkbits); // m_live[i] &= ~chkbits; // m_transients[i] &= ~chkbits;
        Hide
        bryanthompson bryanthompson added a comment -

        AllocationContext.m_active was neither volatile nor an atomic variable. It must be one of these in order to have its state checked without a lock. I have chosen to make it private, final, and an AtomicBoolean.

        Show
        bryanthompson bryanthompson added a comment - AllocationContext.m_active was neither volatile nor an atomic variable. It must be one of these in order to have its state checked without a lock. I have chosen to make it private, final, and an AtomicBoolean.
        Hide
        bryanthompson bryanthompson added a comment - - edited

        AllocationContext.release() will throw an IllegalStateException if the allocation context has already been released. I am not sure that this is a good idea. If this is only called from abort() then that might be Ok. But even then it might be safer to simply ignore the release() call if the allocation context had already been invalidated.

        @martyn please review my changes as of 596d4eaf9fcf5f631cc63b1cab2b5d4352f69c01 in master and also the points above.

        Show
        bryanthompson bryanthompson added a comment - - edited AllocationContext.release() will throw an IllegalStateException if the allocation context has already been released. I am not sure that this is a good idea. If this is only called from abort() then that might be Ok. But even then it might be safer to simply ignore the release() call if the allocation context had already been invalidated. @martyn please review my changes as of 596d4eaf9fcf5f631cc63b1cab2b5d4352f69c01 in master and also the points above.

          People

          • Assignee:
            martyncutcher martyncutcher
            Reporter:
            bryanthompson bryanthompson
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: