search

smack-team / smack

Smack userspace
GNU Lesser General Public License v2.1
41 stars 33 forks source link

Memory optimizations for libsmack #91

Closed rafal-krypa closed 10 years ago

rafal-krypa commented 10 years ago

Two small patches that save 12 bytes per struct smack_rule (on a 32bit system). This difference has significant impact on one of the rule merging approaches that I tried. To have a normalized benchmark, other merging implementations should benefit from it as well.

jarkkojs commented 10 years ago

@rafal-krypa: otherwise looks good but comment about 422b3e8. Using 8-bit integers still take 4 bytes because of natural alignment. Use either int access_code with upper 16-bits deny and lower as 16 allow or two 16-bits words. Either works for me. I would probably just use single integer to encapsulate both codes..

rafal-krypa commented 10 years ago

My commit message for 422b3e8 is surely lacking, it should mention alignment. Pull request topic is also invalid about number of bytes saved. But I have a question about @jsakkine comment: why extend the structure fields to fill internal padding? Using 16-bit fields instead of 8-bit will not change the size of struct, only pad it explicitly on 32-bit machines. When compiled for 64-bits, additional implicit padding will be added by compiler again. What are the benefits of using int16_t instead of int8_t here?

jarkkojs commented 10 years ago

Not much to be honest. Mainly documenting the space usage better, not much else but it does not have any disadvantages either.

jarkkojs commented 10 years ago

@rafal-krypa using int8s works just as well for me, so you can ignore my comment if you want.

jarkkojs commented 10 years ago

I'll just merge these, all review comments so far are too cosmetic to reject this. Meta-note: I updated wiki page for contribution guidelines:

  1. If you have patch that is just small change/fix, you can submit a pull request from any named branch.
  2. If it is an already reported issue, you should change that as a pull request by using hub utility.

For big and non-trivial issues we should probably use mailing list always from now on.

rafal-krypa commented 10 years ago

If it is an already reported issue, you should change that as a pull request by using hub utility.

Two comments about this. As reported before, a submitter must own the issue to convert it to a pull request. So if I hadn't reported the issue initially, I cannot attach patches to it. Secondly, this feature of Github is deprecated since 2013-12-21. The commit message for github/hub.git@4f70dd126f46dec14fc341c97c18efae417743c7 says "This feature is likely to get dropped from GitHub API in the near future."

jarkkojs commented 10 years ago

On Fri, Jan 10, 2014 at 11:12:41AM -0800, Rafał Krypa wrote:

 If it is an already reported issue, you should change that as a pull
 request by using hub utility.

Two comments about this. As reported before, a submitter must own the issue to convert it to a pull request. So if I hadn't reported the issue initially, I cannot attach patches to it. Secondly, this feature of Github is deprecated since 2013-12-21. The commit message for github/hub.git@4f70dd1 says "This feature is likely to get dropped from GitHub API in the near future."

OK. Then we just have separate pull requests and I ease the guidelines so that pull requests can come from any named branch.

/Jarkko