Fast method for ElmsBlist when positions are a range with increment 1 #1773

stevelinton · 2017-10-14T16:59:50Z

Also used for copying pieces of GF2 vectors around.

codecov · 2017-10-14T17:43:22Z

Codecov Report

Merging #1773 into master will decrease coverage by 0.04%.
The diff coverage is 74.41%.

@@            Coverage Diff             @@
##           master    #1773      +/-   ##
==========================================
- Coverage   62.92%   62.87%   -0.05%     
==========================================
  Files         968      968              
  Lines      294059   293097     -962     
  Branches    12987    12929      -58     
==========================================
- Hits       185031   184290     -741     
+ Misses     106227   106016     -211     
+ Partials     2801     2791      -10

Impacted Files	Coverage Δ
src/vecgf2.c	`57.64% <100%> (+1.11%)`	⬆️
src/blister.h	`80.37% <71.64%> (-14.63%)`	⬇️
src/blister.c	`77.14% <76.92%> (-0.28%)`	⬇️
lib/files.gi	`29.09% <0%> (-17.63%)`	⬇️
lib/files.gd	`56.7% <0%> (-5.16%)`	⬇️
src/system.c	`49.31% <0%> (-3.97%)`	⬇️
hpcgap/pkg/gapdoc/PackageInfo.g	`33.75% <0%> (-1.25%)`	⬇️
lib/pager.gi	`7.27% <0%> (-0.61%)`	⬇️
src/c_oper1.c	`87.03% <0%> (-0.57%)`	⬇️
lib/init.g	`43.67% <0%> (-0.5%)`	⬇️
... and 52 more

ChrisJefferson · 2017-10-16T10:12:42Z

While I imagine we'd have realised, some nice test which tests a large range of offsets and lengths would be nice, just to make sure there isn't some nasty corner case somewhere.

stevelinton · 2017-10-16T20:04:34Z

@ChrisJefferson I have a stand along C program which is pretty exhaustive. I'll translate it into GAP.

fingolfin · 2017-10-18T21:05:23Z

I added the "not-for-release-notes" label. But perhaps this PR gives a speed boost to something? Then of course this label should be removed again... but I couldn't tell from the description of the PR.

fingolfin · 2017-10-18T21:12:35Z

src/blister.h

@@ -140,6 +140,9 @@ static inline UInt * BLOCKS_BLIST(Obj list)
    return BLOCKS_BLIST_UNSAFE(list);
 }

+static inline const UInt * CONST_BLOCKS_BLIST(Obj list) {
+  return (const UInt *)BLOCKS_BLIST(list);


I'm afraid this implementation defeats the ultimate purpose: to allow inserting read/write guards in an "optimal" fashion. Really need this (and use 4 spaces for indent):

static inline const UInt * CONST_BLOCKS_BLIST(Obj list) { GAP_ASSERT(IS_BLIST_REP_WITH_COPYING(list)); return ((const UInt *)(CONST_ADDR_OBJ(list) + 1)); }

fingolfin · 2017-10-18T21:15:29Z

src/blister.h

+*/
+
+/* constructs a mask that selects bits <from> to <to> inclusive of a UInt */
+static inline UInt mask(UInt from, UInt to)


Hm, mask is an awfully common name, not sure it's a good idea to use that for a global function.

fingolfin · 2017-10-18T21:33:52Z

src/blister.h

+}
+
+
+static inline  __attribute__((always_inline)) void CopyBits(const UInt * fromblock,


Should use ALWAYS_INLINE from PR #1779

PR #1779 has been merged, so you can now do this.

fingolfin · 2017-10-18T21:37:23Z

src/vecgf2.c

-      }
-    return;
-  }
+  CopyBits(sptr, soff, dptr, doff, nelts);


Have you conducted tests as to whether (and if so: how exactly) this change to the GF2 code affects performance?

stevelinton · 2017-10-19T07:22:31Z

@fingolfin It speeds up ElmsBlist, which was missing this special case, and showed up as 10% of the CPU time in teststandard. More or less equivalent, but much much messier code was already in the vecgf2 applications, so I'd expect little or no change there, but I'll check.

fingolfin · 2017-10-19T09:36:37Z

OK, so it is a performance enhancement. Adjusted labels accordingly.

fingolfin

Some more nitpicks. But the things I really would like to see addressed are: use ALWAYS_INLINE; correct CONST_BLOCKS_BLIST; avoid using misc as name of a globally visible function.

fingolfin · 2017-10-20T12:47:36Z

src/blister.h

+        }
+        /* Now move whole words */
+        if ((wholeblocks = nbits / BIPEB))
+            memcpy((void *)toblock, (void *)fromblock,


I don't think you need the typecasts here (and the second one removes the const qualifier of fromblock.

fingolfin · 2017-10-20T12:49:02Z

src/blister.h

+        *toblock++ = x;
+        nbits -= BIPEB;
+    }
+    /* Finally we may need to fill up a partial block at destination */


Why is that comment attached to the preceding loop, instead of the following if (to which it refers, doesn't it?)

I.e.: maybe put the empty line before the comment, not after it?

fingolfin · 2017-10-20T12:49:55Z

src/vecgf2.c

-      SyExit(2);
-    }
+  soff = (smin-1) %BIPEB;
+  doff = (dmin-1) %BIPEB;


Either add spaces after % or remove the ones before... Or just use clang-format?

(If you add spaces, then perhaps also add them around the / in the next two lines?)

stevelinton · 2017-10-23T11:29:12Z

Added test and addressed all comments, I think

fingolfin

All my original remarks are addressed, and in principle, we could merge this now.

However, I have two more nitpicks, for which I'd appreciate if they could be addressed. That said, if people are in a rush to merge this, go ahead... :-)

fingolfin · 2017-10-23T15:18:12Z

src/blister.h

+    fromblock += frombit / BIPEB;
+    frombit %= BIPEB;
+    toblock += tobit / BIPEB;
+    tobit %= BIPEB;


This function is called from two places, and both already normalize. Is the optimizer clever enough to get rid of the second, redundant normalization?

Otherwise, I'd either remove the normalization in both calling places, or (the approach I personally favor slightly) replace the four lines above by

GAP_ASSERT(frombit < BIPEB); GAP_ASSERT(tobit < BIPEB);

Fine. I put it in because it made it easier to write my C level tests, but you're right it's not needed in GAP.

fingolfin · 2017-10-23T15:19:09Z

src/blister.h

+**  `CopyBits' copies <numbits> bits (numbering bits within a UInt
+**   from the least significant to the most significant) starting with
+**   bit number <from-starting-bit> of UInt *<fromblock> to a destination
+**   starting at bit <to-starting-bit> of *<toblock>.


Are the two blocks assumed to be non-overlapping? I think it would be a good idea to document this explicitly.

…ns is a range of increment 1.

… code that was there before.

fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Oct 18, 2017

fingolfin reviewed Oct 18, 2017

View reviewed changes

fingolfin added this to the GAP 4.9.0 milestone Oct 19, 2017

fingolfin requested changes Oct 20, 2017

View reviewed changes

stevelinton force-pushed the copy-bits branch from fc7bc80 to a4ae227 Compare October 23, 2017 11:20

fingolfin approved these changes Oct 23, 2017

View reviewed changes

stevelinton added 3 commits October 24, 2017 16:10

Introduce CopyBits function and use it for ElmsBlist when the positio…

0e55bc6

…ns is a range of increment 1.

Use the new CopyBits function for GF2 vectors, replacing the horrible…

4e89efc

… code that was there before.

Add test.

4f581a8

stevelinton force-pushed the copy-bits branch from a4ae227 to 4f581a8 Compare October 24, 2017 15:10

markuspf merged commit 144f39c into gap-system:master Oct 27, 2017

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fast method for ElmsBlist when positions are a range with increment 1 #1773

Fast method for ElmsBlist when positions are a range with increment 1 #1773

stevelinton commented Oct 14, 2017

codecov bot commented Oct 14, 2017 •

edited

Loading

ChrisJefferson commented Oct 16, 2017

stevelinton commented Oct 16, 2017

fingolfin commented Oct 18, 2017

fingolfin Oct 18, 2017

fingolfin Oct 18, 2017

fingolfin Oct 18, 2017

fingolfin Oct 20, 2017

fingolfin Oct 18, 2017

stevelinton commented Oct 19, 2017

fingolfin commented Oct 19, 2017

fingolfin left a comment

fingolfin Oct 20, 2017

fingolfin Oct 20, 2017

fingolfin Oct 20, 2017

stevelinton commented Oct 23, 2017

fingolfin left a comment

fingolfin Oct 23, 2017

stevelinton Oct 23, 2017

fingolfin Oct 23, 2017

		}


		static inline __attribute__((always_inline)) void CopyBits(const UInt * fromblock,

Fast method for ElmsBlist when positions are a range with increment 1 #1773

Fast method for ElmsBlist when positions are a range with increment 1 #1773

Conversation

stevelinton commented Oct 14, 2017

codecov bot commented Oct 14, 2017 • edited Loading

Codecov Report

ChrisJefferson commented Oct 16, 2017

stevelinton commented Oct 16, 2017

fingolfin commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelinton commented Oct 19, 2017

fingolfin commented Oct 19, 2017

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelinton commented Oct 23, 2017

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 14, 2017 •

edited

Loading