Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keyLock: add possibility to ask about lock status #3178

Closed
markus2330 opened this issue Nov 8, 2019 · 23 comments · Fixed by #3179
Labels

Comments

@markus2330
Copy link
Contributor

@markus2330 markus2330 commented Nov 8, 2019

In #3170 @manuelm wrote:

Regardless of the exceptions the python bindings still need a way to check if the name has been locked.

What about allowing passing KEY_NAME, KEY_VALUE and KEY_META to keyLock? In this case, keyLock will not lock but simply return the status if it is locked or not.

@markus2330 markus2330 added the proposal label Nov 8, 2019
@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 8, 2019

Very hackish. What about adding a keyTestLock which returns the same value as keyLock but doesn't do anything.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 8, 2019

@manuelm can you find out how much overhead is produced by adding (public) symbols?

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 8, 2019

What exactly do you mean with overhead?

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 8, 2019

Do you mean running with LD_DEBUG=statistics? Are we optimizing cpu cycles now? Symbols get lazy loaded. There's no performance overhead unless the function is actually used. And even then its just one additional lookup.

Consequentially these tests do not show anything: LD_DEBUG=statistics ./bin/test_key

[without keyTestLock]
     32722:
     32722:     runtime linker statistics:
     32722:       total startup time in dynamic loader: 3413068 cycles
     32722:                 time needed for relocation: 2121804 cycles (62.1%)
     32722:                      number of relocations: 1754
     32722:           number of relocations from cache: 1508
     32722:             number of relative relocations: 2006
     32722:                time needed to load objects: 1074424 cycles (31.4%)
     32722:
     32722:     runtime linker statistics:
     32722:                final number of relocations: 1944
     32722:     final number of relocations from cache: 1508

[with keyTestLock defined]
     11271:
     11271:     runtime linker statistics:
     11271:       total startup time in dynamic loader: 3784192 cycles
     11271:                 time needed for relocation: 2134988 cycles (56.4%)
     11271:                      number of relocations: 1754
     11271:           number of relocations from cache: 1508
     11271:             number of relative relocations: 2006
     11271:                time needed to load objects: 1436008 cycles (37.9%)
     11271:
     11271:     runtime linker statistics:
     11271:                final number of relocations: 1944
     11271:     final number of relocations from cache: 1508

[with keyTestLock used]
     13329:
     13329:     runtime linker statistics:
     13329:       total startup time in dynamic loader: 3348768 cycles
     13329:                 time needed for relocation: 2041920 cycles (60.9%)
     13329:                      number of relocations: 1754
     13329:           number of relocations from cache: 1508
     13329:             number of relative relocations: 2006
     13329:                time needed to load objects: 1051632 cycles (31.4%)
     13329:
     13329:     runtime linker statistics:
     13329:                final number of relocations: 1945
     13329:     final number of relocations from cache: 1508
@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 8, 2019

What exactly do you mean with overhead?

The size of the binary. E.g. by making keyVInit static, how much is saved by this?

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 8, 2019

For which century are you optimizing?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 8, 2019

It is still a big topic for embedded systems, e.g. see endless discussions in #2772

And keyLock is basically for binding-writers only, so usability is not so important.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 8, 2019

My first version was bindings only. However you didn't like the approach of including kdbprivat. As they say: sometimes you can't have your cake and eat it.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 8, 2019

It is very possible that the binary size does not increase with more symbols so much. I am interested in how it is, not what we think what it might be.

The "most beautiful" API would be keyLockName, keyIsNameLocked. But then we would get 6 more symbols just for locking. Maybe it is irrelevant, maybe not.

It fits very good in your thesis, as you now did an optimization which reduced the library by one symbol (afaik keyVInit can now be made static).

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 8, 2019

Dynamic symbols are located in .dynstr and .dynsym. The first contains the symbol name (string). The latter contains records of type Elf64_Sym (or Elf32_Sym). Since ELF sections are padded by 4096 bytes (the kernel page size, depends on the architecture) adding or removing a single symbol won't affect the object size. Moreover, the filesystems is allocating in blocks which might increase the file size furthermore.

sizeof(Elf64_Sym) is 24. strlen("keyLockName") is 12 bytes. Therefore, if padding is greater than 36 bytes adding the symbol won't increase the file. Otherwise the file size will increase by 0x1000 bytes.

Maybe it is irrelevant, maybe not.

It's irrelevant.

I am interested in how it is, not what we think what it might be.

Good for you?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 9, 2019

Thank you for looking up the docu. I am be interested if adding these 2 symbols actually hit the padding, i.e., what is the output of ls before and after.

Moreover, the filesystems is allocating in blocks which might increase the file size furthermore.

This we can ignore here.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 10, 2019

To give you a bit more background info: I am totally for having keyIsLocked and keyLock as separate functions, the question for me is if it should be in libelektra-ease or libelektra-core.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 10, 2019

the question for me is if it should be in libelektra-ease or libelektra-core.

I'll add keyLock+keyIsLocked to the cpp bindings, therefore adding the symbols to libelektra-ease will make all users of the cpp binding require to link against ease as well. Not sure you want that.

I am be interested if adding these 2 symbols actually hit the padding, i.e., what is the output of ls before and after.

This is bikeshedding. The actual file size will depend so much on the compiler, the compilers flags and the overall system that two symbols or even 50 more symbols won't matter.

The file size of a library with a single symbol, all irrelevant data stripped, is 6k with gcc 6 vs. 14k with gcc 9. This more than 100% size difference. And you are discussing about two more symbols / potentially +1k…

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 10, 2019

I'll add keyLock+keyIsLocked to the cpp bindings, therefore adding the symbols to libelektra-ease will make all users of the cpp binding require to link against ease as well. Not sure you want that.

No, I do not want that but the cpp binding could also use the low-level interface. But tools and applications written in C could use elektra-ease.

This is bikeshedding.

If the discussion would be only about adding one or two symbols, of course. The discussion is, however, about the API. And the API is the essence of Elektra and is nearly impossible to change afterwards.

One of Elektra's main goal was to be very small and suitable for embedded systems, thus currently the API is partly designed to not use too many symbols (e.g. KDB_O_POP in ksLookup. Some of these decisions were based on comparisons of the library size.). I am open to change that concept (and add also some convenience functions within the core) but I need data for which kind of convenience functions can be in the core, and which not.

I think this discussion would fit well into your thesis, and I think you are an expert for libraries.

And you are discussing about two more symbols / potentially +1k…

If we are speaking about 1k per symbol, it is clearly too much as there are dozens of functions which would need wrappers very much alike keyLock and keyIsLock.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 10, 2019

It's not 1k per symbol. Who said that? It's 24 bytes + strlen(symbol_name) per symbol. Assigned in 1k blocks. e.g one block can hold 50 symbols with every symbol still having a length of 9 bytes (excluding the \0). And that's just a ridiculous 1k.

Edit: Actually this is not 100% correct. Symbol structures and names are in different sections (see above). But I hope you still understand the concept.

I am open to change that concept (and add also some convenience functions within the core) but I need data for which kind of convenience functions can be in the core, and which not.

Yes, you should really change that.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 10, 2019

No, I do not want that but the cpp binding could also use the low-level interface.

So it's allowed for cpp to include kdprivat but not for the swig bindings? The cpp bindings are headers only whereas the swig bindings are binary. This means you need to ship kdbprivat. Don't think this is a good idea in general.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 10, 2019

It's not 1k per symbol. Who said that?

Nobody, it was just an example of what would not be acceptable.

Actually this is not 100% correct.

Of course not. That is why I asked you to measure it. And of course different implementations yield different results.

So it's allowed for cpp to include kdprivat but not for the swig bindings?

No, of course it is also not allowed for the cpp binding, and also not for libelektra-ease. With low-level interface I was referring to keyLock with the 3rd parameter for testing (or 6 different enum values, as I would have preferred). This "low-level interface" in kdb.h could be used by both cpp binding and libelektra-ease. In libelektra-ease we would then also add all other wrappers, to make the bulky kdb.h easier to use, without increasing the size of libelektra-core.

Does this make sense to you?

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 10, 2019

No. It's still bikeshedding. I am not gonna "measure" how much the file size changes if I add/remove one single symbol as I do think it shouldn't affect this decision and the results are way to much depending on the platform, your tools and the already exported symbols. I told you how it's implemented. Thats way more accurat than your measurements. Ofc. you can do whatever you want.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 10, 2019

No. It's still bikeshedding.

What is not? If type names end with _t? If we require C99? If we return a pointer to our data structure or copy the content instead? If we use vaargs?

At the moment libelektra-core only contains symbols that cannot be implemented outside the core. For me it sounds like a quite good criteria. In #3179 this rule is broken. I asked for a measurement of the impact of changing this rule (and allow trivial wrapper).

I am not gonna "measure" how much the file size changes if I add/remove one single symbol as I do

Nobody asked you to do this for the purpose of measuring only this. Of course ideally the measurement should be designed to answer the question if it is safe to add all wrappers within the core and what is the criteria that a wrapper should be better in libelektra-ease.

think it shouldn't affect this decision

What should? In most cases it is not a big deal to link against libelektra-ease, so wrappers can be there.

the results are way to much depending on the platform, your tools and the already exported symbols.

Yes, it would be good if more than one platform is covered. ELF is also not the only way to implement libraries.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 10, 2019

I dont want to argue any more. I'll ignore any requests related to binary size.

Are you fine with keyLockEx(key, options, test) + 2 defines? Its not that hackish as your initial idea. Or we simply call it keyLock with 3 arguments. Both will only add one single symbol. The code is already part of core anyway.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 11, 2019

I made the test myself, it is 73040 vs. 73032 bytes on amd64 after strip. So it really does not matter and the difference is much less than we anticipated. (317592-316920=672 bytes on unstripped).

So we can go with keyLock and keyIsLock.

Two more request for #3179:

  • please move the implementation away from src/libs/proposal/proposal.c as we want to get rid of this file (strictly speaking keyIsLock should be in
  • make elektraKeyLock and keyVInit static as they should not be a symbol to be used from outside.

Are you fine with keyLockEx(key, options, test) + 2 defines?

I was okay with the 3 parameters. Macros can be, if really needed, in kdbmacros.h.

The "Ex", however, seems to be unnecessary/weird. Is it for Extended?

Its not that hackish as your initial idea.

3 parameters have even more possibilities to extend functionality. It invites for very hacky extensions.

Or we simply call it keyLock with 3 arguments.

Definitely better than keyLockEx.

Both will only add one single symbol. The code is already part of core anyway.

Actually it would not add a symbol, as elektraKeyLock was already a symbol before, it would simply be renamed.

@manuelm

This comment has been minimized.

Copy link
Contributor

@manuelm manuelm commented Nov 11, 2019

please move the implementation away from src/libs/proposal/proposal.c

Do you mean src/libs/*elektra*/proposal.c? (I marked the path difference)

@markus2330

This comment has been minimized.

Copy link
Contributor Author

@markus2330 markus2330 commented Nov 11, 2019

Yes, sorry for the confusion, elektraKeyLock is in src/libs/elektra/proposal.c

src/libs/proposal/proposal.c should be also removed but this is a different issue #2993

@PhilippGackstatter PhilippGackstatter referenced this issue Nov 12, 2019
3 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.