Speeding up C implementation #42

dearblue · 2023-07-13T14:49:05Z

Switch from Sarwate's algorithm to byte-order free Slicing-by-16.
Each table data file is generated by extconf.rb.

In addition, the inspection string, which was previously 10 bytes, is extended to 19 bytes.
This is to check the slicing-by-16 loop, which processes in 16-byte units.

dearblue · 2023-07-13T14:50:56Z

For information, here is a summary of the benchmark results.
The environment is the same as #41 (comment).

CRC Models	main	slicing-by-8	slicing-by-16
Digest::CRC1#update	( 0.394643)	( 0.404651)	( 0.400259)
Digest::CRC5#update	( 0.015706)	( 0.004514)	( 0.003536)
Digest::CRC8#update	( 0.013652)	( 0.004703)	( 0.003673)
Digest::CRC8_1Wire#update	( 0.013575)	( 0.004455)	( 0.003519)
Digest::CRC15#update	( 0.020950)	( 0.004627)	( 0.003702)
Digest::CRC16#update	( 0.018752)	( 0.004552)	( 0.003753)
Digest::CRC16CCITT#update	( 0.019875)	( 0.004606)	( 0.003683)
Digest::CRC16DNP#update	( 0.016596)	( 0.004538)	( 0.003724)
Digest::CRC16Genibus#update	( 0.019800)	( 0.004601)	( 0.003699)
Digest::CRC16Modbus#update	( 0.018721)	( 0.004683)	( 0.003724)
Digest::CRC16QT#update	( 0.018728)	( 0.004732)	( 0.003727)
Digest::CRC16USB#update	( 0.018706)	( 0.004654)	( 0.003923)
Digest::CRC16X25#update	( 0.020320)	( 0.004558)	( 0.003866)
Digest::CRC16XModem#update	( 0.025078)	( 0.004610)	( 0.003897)
Digest::CRC16ZModem#update	( 0.022976)	( 0.004603)	( 0.003760)
Digest::CRC24#update	( 0.021261)	( 0.004867)	( 0.003787)
Digest::CRC32#update	( 0.017693)	( 0.004533)	( 0.003643)
Digest::CRC32BZip2#update	( 0.017650)	( 0.004801)	( 0.003785)
Digest::CRC32c#update	( 0.017690)	( 0.004551)	( 0.003679)
Digest::CRC32Jam#update	( 0.017696)	( 0.004685)	( 0.003636)
Digest::CRC32MPEG#update	( 0.017663)	( 0.004937)	( 0.003790)
Digest::CRC32POSIX#update	( 0.017704)	( 0.004839)	( 0.003791)
Digest::CRC32XFER#update	( 0.017731)	( 0.004858)	( 0.003827)
Digest::CRC64#update	( 0.017776)	( 0.004971)	( 0.004435)
Digest::CRC64Jones#update	( 0.017718)	( 0.004897)	( 0.004274)
Digest::CRC64XZ#update	( 0.017697)	( 0.004946)	( 0.004311)

Finally, FreeBSD uses clang, so if built with GCC 13, the results will be different.
Only the CRC-32 results are excerpted.

% gmake -C ext/digest/crc CC=gcc13 'cflags=$(optflags) $(debugflags) $(warnflags)' 'warnflags=-Wall -Wextra -Wundef' clean all

gcc13 sarwate
Digest::CRC32#update          0.017745   0.000000   0.017745 (  0.017761)

gcc13 slicing-by-8
Digest::CRC32#update          0.003948   0.000000   0.003948 (  0.003921)

gcc13 slicing-by-16
Digest::CRC32#update          0.002824   0.000000   0.002824 (  0.002815)

postmodern

There's a lot going on here which I worry will make it harder to maintain and for others to understand. I would prefer that each generated C extension be fully explicit and self-contained, allowing them to be loaded individually and easily be reviewed/audited.

postmodern · 2023-07-17T04:35:28Z

ext/digest/crc12_3gpp/crc12_3gpp_ext.c

@@ -27,6 +16,5 @@ void Init_crc12_3gpp_ext()
 		rb_ext_ractor_safe(true);
 	#endif

-	rb_undef_method(cCRC12_3GPP, "update");
-	rb_define_method(cCRC12_3GPP, "update", Digest_CRC12_3GPP_update, 1);


The reason why we undef and re-define the update method is to prevent warnings from ruby -w, which some people apparently use for testing/production. This solution will need to keep this, or prove it doesn't cause warnings under ruby -w.

I will fix it to do undef.

postmodern · 2023-07-17T04:37:55Z

ext/digest/crc12_3gpp/extconf.rb


 have_header("stdint.h")
 have_header('stddef.h')

+slice_size = ENV["RUBY_DIGEST_CRC_ENABLE_SLICING_BY_16"].to_i > 0 ? 16 : 8


Could we use if/else with an implicit return value instead of ternary expressions? Also could we use shorter environment variable names, like RUBY_DIGEST_CRC_SLICE_SIZE?

postmodern · 2023-07-17T04:41:52Z

ext/digest/crc16_dnp/extconf.rb


 have_header("stdint.h")
 have_header('stddef.h')

+slice_size = ENV["RUBY_DIGEST_CRC_ENABLE_SLICING_BY_16"].to_i > 0 ? 16 : 8


Prefer to use if/else with implicit return values over ternary expressions.

postmodern · 2023-07-17T04:43:12Z

ext/digest/crc16_kermit/extconf.rb


 have_header("stdint.h")
 have_header('stddef.h')

+slice_size = ENV["RUBY_DIGEST_CRC_ENABLE_SLICING_BY_16"].to_i > 0 ? 16 : 8


Prefer to use if/else with an implicit return value instead of ternary expressions. Also this logic seems to be repeated in each file. Maybe it should be moved into gentable.rb?

Maybe it should be moved into gentable.rb?

Yes, I will.

postmodern · 2023-07-17T04:45:33Z

ext/digest/gentable.rb

@@ -0,0 +1,125 @@
+using Module.new {


Let's not use refinements or monkey-patching here. I think a regular method named bitreflrect() is good enough.

postmodern · 2023-07-17T04:59:39Z

ext/digest/crc/crc_ext.c

+	\
+	for (int i = 0; i < 256; i++) { \
+		crc_int_t n = V(table_initialize)(model->bitwidth, i); \
+		for (int j = 0; j < 8; j++) { \


postmodern · 2023-07-17T04:59:45Z

ext/digest/crc/crc_ext.c

+		table[0][i] = n; \
+	} \
+	\
+	for (int s = 1; s < SLICE_SIZE; s++) { \


postmodern · 2023-07-17T05:00:06Z

ext/digest/crc/crc_ext.c

+	} \
+	\
+	for (int s = 1; s < SLICE_SIZE; s++) { \
+		for (int i = 0; i < 256; i++) { \


postmodern · 2023-07-17T05:03:53Z

ext/digest/crc.h

+#if !defined(HAVE_RB_EXT_RACTOR_SAFE)
+static inline void ruby_digest_crc_ensure_ractor_main(const char *mesg)
+{
+	(void)mesg;


Not sure what the purpose is of this statement? Seems to be a no-op?

This is used to suppress cc -Wextra warnings about unused variables.

postmodern · 2023-07-17T05:06:01Z

ext/digest/crc/crc_ext.c

+#define BUILD_TABLE_CORE_DECL(V) \
+	const crc_int_t modified_poly = V(adapt)(model->bitwidth, model->polynomial); \
+	\
+	for (int i = 0; i < 256; i++) { \


Also an old school optimization trick is to always define your loop variables above/outside of your loop. This prevents dumb compilers for re-allocating space on the stack for nested for loop index variables.

dearblue · 2023-07-24T13:15:47Z

Summary of changes:.

Dedicated functions for each CRC are now provided.
No longer provides generic functions.
The CRC-64 table is now 32 KiB, but I believe modern PC processors have L1 data caches of 64 KiB or more.
Extend spec check string to 19 bytes (also in commit message).

Tables are still generated at build time of the installation.
As I wrote to #42 (comment), including the tables in the repository will probably increase the GEM package size to around 400 KiB.

dearblue · 2023-07-24T13:22:44Z

ext/digest/crc12_3gpp/crc12_3gpp.c

@@ -15,52 +15,42 @@
 */


According to https://github.com/tpircher/pycrc#copyright-of-the-generated-source-code, the generated code can be treated as public domain.
Do we need any addenda as a digest-crc project to prevent misidentification?

🐈‍⬛ I can't see it because it's out of sight.

digest-crc/ext/digest/crc12_3gpp/crc12_3gpp.c

Lines 1 to 15 in 91ee75f

/**

* \file

* Functions and types for CRC checks.

*

* Generated on Sat Feb 29 02:30:42 2020

* by pycrc v0.9.2, https://pycrc.org

* using the configuration:

* - Width = 12

* - Poly = 0x80f

* - XorIn = 0x000

* - ReflectIn = False

* - XorOut = 0x000

* - ReflectOut = True

* - Algorithm = table-driven

*/

Probably not. We may need to change the wording to "Originally generated" or "Derived from", since I made some modifications by adding custom typdefs.

😺 I'll not do anything in this PR as I believe you will make this change if necessary.

Switch from Sarwate's algorithm to byte-order free Slicing-by-16. Each table data file was generated by `ext/digest/gentable.rb`. In addition, the inspection string for rspec, which was previously 10 bytes, is extended to 19 bytes. This is to check the Slicing-by-16 loop, which processes in 16-byte units.

dearblue · 2023-08-15T05:20:22Z

Table data is to be prepared in advance.
However, ext/digest/gentable.rb will be kept for future extensions.

Also, I forgot to mention in the last change, "Slicing-by-16" is always used.
This is because I don't know how to write a test that switches to "Slicing-by-8".

postmodern requested changes Jul 17, 2023

View reviewed changes

dearblue force-pushed the slicing-by branch from 62e6d2b to e6f0aa3 Compare July 24, 2023 12:54

dearblue commented Jul 24, 2023

View reviewed changes

dearblue force-pushed the slicing-by branch from e6f0aa3 to f93262b Compare August 15, 2023 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speeding up C implementation #42

Speeding up C implementation #42

dearblue commented Jul 13, 2023 •

edited

Loading

dearblue commented Jul 13, 2023

postmodern left a comment

postmodern Jul 17, 2023

dearblue Jul 17, 2023

postmodern Jul 17, 2023

postmodern Jul 17, 2023

postmodern Jul 17, 2023

dearblue Jul 17, 2023

postmodern Jul 17, 2023

postmodern Jul 17, 2023

postmodern Jul 17, 2023

postmodern Jul 17, 2023

postmodern Jul 17, 2023

dearblue Jul 17, 2023

postmodern Jul 17, 2023

dearblue commented Jul 24, 2023

dearblue Jul 24, 2023

dearblue Jul 24, 2023

postmodern Jul 24, 2023

dearblue Aug 15, 2023

dearblue commented Aug 15, 2023

	/**
	* \file
	* Functions and types for CRC checks.
	*
	* Generated on Sat Feb 29 02:30:42 2020
	* by pycrc v0.9.2, https://pycrc.org
	* using the configuration:
	* - Width = 12
	* - Poly = 0x80f
	* - XorIn = 0x000
	* - ReflectIn = False
	* - XorOut = 0x000
	* - ReflectOut = True
	* - Algorithm = table-driven
	*/

Speeding up C implementation #42

Are you sure you want to change the base?

Speeding up C implementation #42

Conversation

dearblue commented Jul 13, 2023 • edited Loading

dearblue commented Jul 13, 2023

postmodern 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

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

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

dearblue commented Jul 24, 2023

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

dearblue commented Aug 15, 2023

dearblue commented Jul 13, 2023 •

edited

Loading