Optimizations #123

pbfy0 · 2018-05-13T19:13:28Z

This pull requests contains several optimizations, both for speed and for code size.

Moves the actual definitions in keys.h to its own source file. This makes both libndls and ndless_resources.tns significantly (~30kb) smaller, because both of them contained several copies of the table before.
Default to looking for ndless.cfg in get_documents_dir()/ndless/ndless.cfg.tns, and cache its last seen location in memory if it's not found there. Closes Repeated searching for ndless.cfg is extremely slow if you have a lot of documents #120.
After locating programs for file associations, store their full paths back in ndless.cfg.tns. If the program is moved from the stored path, ndless will find the new location and store it. If the program is deleted, ndless will revert the config entry back to a basename only.

… ndless_resources

As a result, ndless will write the location it has found the associated program to ndless.cfg. This significantly speeds opening programs with association, since it avoids trawling through the entire filesystem each time.

Vogtinator · 2018-05-13T20:17:42Z

ndless-sdk/libndls/config.c

 }

+static char cfg_path[FILENAME_MAX] = {0};


Move into the function itself

Vogtinator · 2018-05-13T20:25:30Z

ndless-sdk/include/libndls.h

@@ -102,10 +102,23 @@ BOOL isKeyPressed(const t_key *key);
 void wait_key_pressed(void);
 void wait_no_key_pressed(void);
 /* config.c */
+struct cfg_entry {
+	char key[15];
+//	short val_offset;


Vogtinator · 2018-05-13T20:25:49Z

ndless-sdk/include/libndls.h

@@ -102,10 +102,23 @@ BOOL isKeyPressed(const t_key *key);
 void wait_key_pressed(void);
 void wait_no_key_pressed(void);
 /* config.c */
+struct cfg_entry {
+	char key[15];


Fixed size?

I thought this was OK because the only place that wrote to the cfg in code (cfg_register_fileext) also limited the key size to 15. However, this doesn't take into account manual editing of the config file

Yes, for the fileext it might apply, but not for other keys. Part of this effort of refactoring this is to make the code more versatile, isn't it? :-)

How would one make this a non-static size? A variable and a helper function?

Most likely, yes. Alternatively make the fixed size bigger and provide it as a global constant.

Vogtinator · 2018-05-13T20:32:59Z

ndless-sdk/libndls/config.c

 }

 void cfg_open(void) {
-	char path[300];
+	char path[FILENAME_MAX];


I saw FILENAME_MAX used in this way in a few other places in ndless (i.e. in ld_exec_with_args). Doesn't make it right, but that was my logic.

Vogtinator · 2018-05-13T20:36:18Z

ndless-sdk/libndls/config.c

-		if (!strcmp(key, kv_offsets[i][0] + file_content))
-			return kv_offsets[i][1] + file_content;
+		struct cfg_entry *e = &cfg_entries[i];
+		//printf("%s, %s\n", key, e->key);


Debug statement, remove.

Vogtinator · 2018-05-13T20:36:57Z

ndless-sdk/libndls/config.c

-			return kv_offsets[i][1] + file_content;
+		struct cfg_entry *e = &cfg_entries[i];
+		//printf("%s, %s\n", key, e->key);
+		if (!strcmp(key, e->key)) {


Remove unnecessary {}, use == 0.

Vogtinator · 2018-05-13T20:37:52Z

ndless-sdk/libndls/config.c

+
+void cfg_put(const char *key, const char *val) {
+	struct cfg_entry *entr;
+	if((entr = cfg_get_entry(key))) {


Move the assignment outside of the if.

Vogtinator · 2018-05-13T20:38:51Z

ndless-sdk/libndls/config.c

+	return e == NULL ? NULL : e->value; 
+}
+
+static int strict_strcmp(const char *a, const char *b) {


What does this do?

Normal strcmp returns zero if the second string starts with the first string. This version doesn't.

It doesn't. strcmp("foo", "foobar") = -1

You're right. I tested with gcc.godbolt.org and misread mvn as mov.

Vogtinator · 2018-05-13T20:39:17Z

ndless-sdk/libndls/config.c

+	return memcmp(a, b, (l1 < l2 ? l1 : l2)+1);
+}
+
+void cfg_put_entry(struct cfg_entry *entr, const char *val) {


This API doesn't make sense - struct cfg_entry already has a value.

The point of this is to let config.c only write out the config if a change has actually happened. Maybe it would be better to make struct cfg_entry opaque outside of config.c. cfg_put_entry isn't a great name, I guess. Maybe cfg_set_value?

Yes, cfg_entry should be opaque.

Vogtinator · 2018-05-13T20:42:52Z

Moves the actual definitions in keys.h to its own source file. This makes both libndls and ndless_resources.tns significantly (~30kb) smaller, because both of them contained several copies of the table before.

I wonder why the compiler/linker did not optimize it. It's just a bunch of unused variables. Did you look for it with objdump?

Default to looking for ndless.cfg in get_documents_dir()/ndless/ndless.cfg.tns, and cache its last seen location in memory if it's not found there. Closes #120.

Looks good to me.

After locating programs for file associations, store their full paths back in ndless.cfg.tns. If the program is moved from the stored path, ndless will find the new location and store it. If the program is deleted, ndless will revert the config entry back to a basename only.

I don't like this too much TBH. It adds more complexity to the code for no good reason. Just use the full path in the cfg? ndless should only ever read the config, not change it.

pbfy0 · 2018-05-13T21:20:54Z

For keys.h, I did look at the objdump; I did see seven copies of KEY_NSPIRE_xxx in one of my projects.
The config changes are pretty complicated. Maybe it would be better to have a separate file, managed by ndless, to map program names to file locations?

pbfy0 · 2018-05-13T21:26:23Z

On the other hand, having a better API to manage config entries might be a good thing. cfg_register_fileext currently does nothing if the config entry already exists; this is a little unsatisfactory.

Vogtinator · 2018-05-13T21:45:31Z

For keys.h, I did look at the objdump; I did see seven copies of KEY_NSPIRE_xxx in one of my projects.
The config changes are pretty complicated. Maybe it would be better to have a separate file, managed by ndless, to map program names to file locations?

I'd say that the cfg file should just contain absolute paths when possible.

On the other hand, having a better API to manage config entries might be a good thing. cfg_register_fileext currently does nothing if the config entry already exists; this is a little unsatisfactory.

I agree.

It should be done in separate steps though, with support for absolute paths in a separate commit from the cfg_ API refactoring. Maybe even split it into separate PRs so that it can be merged independently.

Vogtinator · 2018-05-13T21:51:58Z

For keys.h, I did look at the objdump; I did see seven copies of KEY_NSPIRE_xxx in one of my projects.

Seven copies should not be more than a handful of bytes, or were all available key_t listed?

pbfy0 · 2018-05-13T22:06:31Z

All available keys. As I said, it was ~30kb. Not huge, but not negligible either

Vogtinator · 2018-05-14T07:07:01Z

All available keys. As I said, it was ~30kb. Not huge, but not negligible either

Can you give a (code) example of how it was used? I suspect a huge array of pointers, but then the bug is in that application IMO.

pbfy0 · 2018-05-14T22:43:24Z

Turns out that one is me being stupid; it was a result of compiling with -O0. With any other level of optimization, gcc removes the unused copies.

adriweb · 2018-08-12T07:36:35Z

@pbfy0 any update?

adriweb · 2020-06-14T16:36:07Z

We're sad to learn that @pbfy0 passed away in December 2019 (see link in his profile)...

Not sure how to help handling this PR, especially as it hadn't been updated since mid-2018. Does someone want to work-on/rebase his commits and add more to take care of @Vogtinator's comments? And of course, keeping authorship for the original commits would be nice and not a problem git-wise.

gameblabla · 2020-11-02T00:55:28Z

I made my own attempt here, although i wasn't able to keep the authorship of the original commits : (
https://github.com/gameblabla/Ndless/commits/opts

I need to test it though.

EDIT: Ok so it compiles after i made a few changes but still not sure if it works. Should i open a separate pull request ?

pbfy0 added 4 commits April 29, 2018 18:06

keys in a C file so we don't end up with seven copies of the table in…

5ea35e9

… ndless_resources

Don't search filesystem for ndless.cfg every time

8cafb6e

support absolute paths in ndless.cfg

134ae5e

Vogtinator requested changes May 13, 2018

View reviewed changes

removed debugging prints

e71bc41

pbfy0 added 2 commits May 13, 2018 17:40

style changes, use PATH_MAX rather than FILENAME_MAX

757349d

moved cfg_path inside fuction, removed old struct member

93ae8c3

renamed cfg_put_entry, removed strict_strcmp, fixed writing comments

9c9c119

gameblabla mentioned this pull request Nov 14, 2020

Optimizations by pbfy0 (Rebase, needs testing) #255

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimizations #123

Optimizations #123

pbfy0 commented May 13, 2018

Vogtinator May 13, 2018

Vogtinator May 13, 2018

Vogtinator May 13, 2018

pbfy0 May 13, 2018

Vogtinator May 14, 2018

Arudarin Aug 16, 2019

Vogtinator Aug 16, 2019

Vogtinator May 13, 2018

pbfy0 May 13, 2018

Vogtinator May 13, 2018

Vogtinator May 13, 2018

Vogtinator May 13, 2018

Vogtinator May 13, 2018

pbfy0 May 13, 2018

Vogtinator May 13, 2018

pbfy0 May 14, 2018

Vogtinator May 13, 2018

pbfy0 May 13, 2018

Vogtinator May 13, 2018 •

edited

Loading

Vogtinator commented May 13, 2018

pbfy0 commented May 13, 2018

pbfy0 commented May 13, 2018

Vogtinator commented May 13, 2018

Vogtinator commented May 13, 2018 •

edited

Loading

pbfy0 commented May 13, 2018

Vogtinator commented May 14, 2018

pbfy0 commented May 14, 2018

adriweb commented Aug 12, 2018

adriweb commented Jun 14, 2020 •

edited

Loading

gameblabla commented Nov 2, 2020 •

edited

Loading

Optimizations #123

Are you sure you want to change the base?

Optimizations #123

Conversation

pbfy0 commented May 13, 2018

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

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

Vogtinator May 13, 2018 • edited Loading

Choose a reason for hiding this comment

Vogtinator commented May 13, 2018

pbfy0 commented May 13, 2018

pbfy0 commented May 13, 2018

Vogtinator commented May 13, 2018

Vogtinator commented May 13, 2018 • edited Loading

pbfy0 commented May 13, 2018

Vogtinator commented May 14, 2018

pbfy0 commented May 14, 2018

adriweb commented Aug 12, 2018

adriweb commented Jun 14, 2020 • edited Loading

gameblabla commented Nov 2, 2020 • edited Loading

Vogtinator May 13, 2018 •

edited

Loading

Vogtinator commented May 13, 2018 •

edited

Loading

adriweb commented Jun 14, 2020 •

edited

Loading

gameblabla commented Nov 2, 2020 •

edited

Loading