Cleanup repo sources, tests, Apply pylint. Update README.md by gapisback · Pull Request #2 · undoio/l3

gapisback · 2024-03-03T14:18:29Z

This commit does code-cleanup for diff files in the core l3 package.

Rewrite README as README.md. Update / correct description
Makefile: Add blank lines for readability. Run pylint on l3_dump.py . Invoke script to exercise help output.
l3.h - Add code comments. Mark params as const where applicable.
l3.c - Code tightening: - Consistently enclose all single-line bodies in { } with indentation - Replace constants with mnemonics; e.g. L3_DLADDR_NOT_FOUND_ERRNO - Rename mytid() -> l3_mytid(), to namespace the lookup fn.
l3_dump.py - Address all pylint errors (now runs in Make) Add copyright notice to Py script.
test.c - Various test cleanups. Add timespec_to_ns()

Tested on Ubuntu Linux-VM.

Sample output from dev-test on my Ubuntu-VM:

agurajada-Linux-Vm:[100] $ make test
pylint l3_dump.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

gcc -Wall -o test -g -O3 test.c l3.c l3.S
./test

Exercise in-memory logging performance benchmarking: 300 Mil simple/fast log msgs
300 Mil simple log msgs: 9ns/msg
300 Mil fast log msgs  : 8ns/msg

./l3_dump.py
Usage: ./l3_dump.py <logfile> <program-binary>

python3 l3_dump.py /tmp/l3_test ./test
tid=44459 '300-Mil Fast l3-log msgs' arg1=0 arg2=0
tid=44459 '300-Mil Fast l3-log msgs' arg1=0 arg2=0
tid=44459 '300-Mil Fast l3-log msgs' arg1=0 arg2=0

python3 l3_dump.py /tmp/l3_small_test ./test
tid=44459 'Simple-log-msg-Args(1,2)' arg1=1 arg2=2
tid=44459 'Potential memory overwrite (addr, size)' arg1=3735927486 arg2=1024
tid=44459 'Invalid buffer handle (addr)' arg1=3203378125 arg2=0

gapisback · 2024-03-03T14:21:23Z

 	./test
-	python l3_dump.py /tmp/l3_test test
-	python l3_dump.py /tmp/l3_small_test test
+	@echo


Add blank lines between every new command run, so the chunks of output are more visible in stdout.

Also on L9 below, we run the Py script w/o args, just to be sure that the help/usage message is still coming out correctly.

gapisback · 2024-03-03T14:21:51Z

@@ -0,0 +1,51 @@
+# L3: The Lightweight Logging Library


Rewrote in md format, with minor inline edits and rewrites for better readability.

gapisback · 2024-03-03T14:22:43Z

+ * \version 0.1
+ * \date 2023-12-24
+ *
+ * \copyright Copyright (c) 2023


Should the Copyright messages in diff files change to: 2023-2024?

That kind of annual update is a nightmare ... How does one manage that in other oss repos?

gapisback · 2024-03-03T14:23:22Z


+/**
+ * L3 Log Structure definitions:
+ */


Add small prologs to each struct describing what it stands for.

Please correct or add more descriptive text if you wish to see some.

gapisback · 2024-03-03T14:24:54Z

-        if (fd == -1) return -1;
+        if (fd == -1) {
+            return -1;
+        }


I strongly encourage and request that we should adhere to the practice of enclosing even single-line if(), while() bodies inside { }and conform to proper indentation on the next line.

I have applied this change to few files.

Long-term: We should consider standardizing on a clang-format format-file, run that in CI, so all new code comes out looking consistent.

gapisback · 2024-03-03T14:26:08Z

+L3: Lightweight Logging Library, Version 0.1
+Date 2023-12-24
+Copyright (c) 2023-2024
+"""


Applied pylint to this file, and that's included in the Makefile, too.

Future changes have to conform otherwise builds will fail.

gapisback · 2024-03-03T14:27:03Z

+    rodata_offs = int(stdout.split('\n')[2].split()[0], 0)
+
+# pylint: disable-next=line-too-long
+with subprocess.Popen(["readelf", "-p", ".rodata", sys.argv[2]], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) as p:


Rewrite here and on L17 above to enclose statement using with was suggested by pylint for reasons of correct resource mgmt.

Another benefit of running pylint.

gapisback · 2024-03-03T14:27:30Z

+static uint64_t inline
+timespec_to_ns(struct timespec *ts)
+{
+    return ((ts->tv_sec * L3_NS_IN_SEC) + ts->tv_nsec);


Modularized into tiny helper fn.

gapisback · 2024-03-03T14:28:00Z


+// Useful constants
+#define L3_MILLION      (1000 * 1000)
+#define L3_NS_IN_SEC    (1000 * 1000 * 1000)


Add mnemonics to recognize eye-blurring mind-numbing constants with series of 0s.

gapisback · 2024-03-03T14:28:48Z


+    int nMil = 300;
+    printf("\nExercise in-memory logging performance benchmarking: "
+           "%d Mil simple/fast log msgs\n",


Msg now reports workloads' #-of-inserts parameter.

gapisback · 2024-03-03T14:29:20Z


-    printf("simple: %" PRIu64 "ns\n", (nsec1 - nsec0) / n);
+    printf("%d Mil simple log msgs: %" PRIu64 "ns/msg\n",
+            nMil, (nsec1 - nsec0) / n);


Clarify the units of the metric reported. Mention that it's avg.

One day, we may want to track and report min / avg / max / std-dev metrics, for proper benchmarking results. Future.

This commit does code-cleanup for diff files in the core l3 package. - Rewrite README as README.md. Update / correct description - Makefile: Add blank lines for readability. Run `pylint` on l3_dump.py . Invoke script to exercise help output. - l3.h - Add code comments. Mark params as const where applicable. - l3.c - Code tightening: - Consistently enclose all single-line bodies in { } with indentation - Replace constants with mnemonics; e.g. L3_DLADDR_NOT_FOUND_ERRNO - Rename mytid() -> l3_mytid(), to namespace the lookup fn. - l3_dump.py - Address all pylint errors (now runs in Make) Add copyright notice to Py script. - test.c - Various test cleanups. Add timespec_to_ns() - Report units of throughput metric. Tag as 'avg'. Tested on Ubuntu Linux-VM.

gregthelaw

Excellent - thank you for these changes. All good!

gapisback commented Mar 3, 2024

View reviewed changes

gapisback force-pushed the gapisback/code-cleanup branch from 629f4dc to aa3797c Compare March 3, 2024 14:31

gapisback requested a review from gregthelaw March 3, 2024 14:32

gregthelaw approved these changes Mar 3, 2024

View reviewed changes

gregthelaw merged commit 2e5fc6d into main Mar 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup repo sources, tests, Apply pylint. Update README.md#2

Cleanup repo sources, tests, Apply pylint. Update README.md#2
gregthelaw merged 1 commit intomainfrom
gapisback/code-cleanup

gapisback commented Mar 3, 2024 •

edited

Loading

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024

Uh oh!

gapisback Mar 3, 2024 •

edited

Loading

Uh oh!

gregthelaw left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gapisback commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Sample output from dev-test on my Ubuntu-VM:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gapisback Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregthelaw left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gapisback commented Mar 3, 2024 •

edited

Loading

gapisback Mar 3, 2024 •

edited

Loading