On Fri, Jul 27, 2012 at 5:25 PM, Jakub Jermar <[email protected]> wrote: > Hi Adam, > > I plan to have a thorough look at your recent changes during my back > flight from Black Hat; at this point I have just a couple of comments, > see below.
Great :-). > On 07/24/2012 04:58 PM, Adam Hraska wrote: >> 2) I got rid of another hash table implementation in >> uspace/*/adt/hash_set and switched its only user >> nic_addr_db to hash_table. > > Is networking still working with one of the nic drivers?? How would I go about testing that? I tried: / # ping 127.0.0.1 / # netecho / # nettest1 / # nettest2 .. and at this point netecho (or nettest2) crash (assert(false) in sock.c:371). However, these tests behaved the same way even before I made any changes. Note, that I started helenos in qemu: $ qemu -cdrom image.iso -smp 4 Wiki contains these network related guides: http://trac.helenos.org/wiki/UsersGuide/RunningInQEMU#Networking http://trac.helenos.org/wiki/UsersGuide/Networking http://trac.helenos.org/wiki/NetworkBridging >> Details: >> (1) required a slight change of the interface, so >> many files were affected by the commit. > > The original hash table supported a trick: the compare function could > have been passed fewer keys for a partial match, which has been used in > hash_table_remove() to remove more partially matching keys at once. I > noticed you tried to preserve this behaviour, but just wanted to point > out it was there and that it might have been used. Yeah, I have noticed this feature. IIRC it has been used in 2 or 3 places out of the total of ~25 hash tables in uspace. Actually I was thinking of replacing the special purpose hash_table_remove(partial-match) with the more general hash_table_apply() (it already has some users) since there are only few users of the partial matching remove(). Moreover, the partial matching remove() cannot be implemented efficiently, so using hash_table_apply() instead might make that fact explicit -- that the whole table must be traversed. >> (1) I did not know how to thoroughly test the change, >> so I copied ~15000 files back and forth from fatfs >> to tmpfs. I also ran the user tester tests a couple >> of times. > > In your tests, you could also include networking involving NIC drivers, > try to mount & unmount some FAT and TMPFS file systems that have some > files on them. I already confessed that I have no idea how to test the NIC drivers. Now is the right time to admit that I have tried to mount a FAT fs and failed. I mimicked: http://trac.helenos.org/wiki/CoreFiles in order to create a FAT volume and add it to qemu. Somehow, helenos did not recognize the new hdd and I did not figure out how to mount it (ie what to supply to mount). Moreover, ls /loc/devices and ls /lod/bd had the same output as when running in qemu without the hdd attached (ie without the -hda qemu switch). >> (1) The single-threaded hash table now uses table sizes >> that are (mostly) prime to protect against suboptimal >> hashes. Its minimum size is 89 buckets. If someone >> wanted to create many small hash tables and the minimum >> table size would waste too much memory, I can switch >> the table from computing its new size to looking it up >> in a static table of primes. That would allow the table >> to have a size less than 89. > > I noticed that for each converted hash() function, there would be some > boilerplate code matching the following pattern: > > /* Inspired by Effective Java, 2nd edition. */ > size_t hash = 17; > > hash = 31 * hash + key[0]; > ... > hash = 31 * hash + key[n]; > > Could we do better and turn this into some kind of a macro or even a > function (for each reasonably small n) so that we don't have this > comment and code in a dozen of locations? I fully agree. It took me "only" 10 identical hash function implementations to realize this is not the way to go ;-). I added a general hash combining function hash_combine() to adt/hash.h. I will convert existing hash functions to it when time permits. Btw, it would have been awesome if command help messages also included a small usage example, eg: ping 127.0.0.1 .. pings the local host Sometimes it is a bit difficult for a novice such as myself to decipher what exactly is <dev> and <moptions> in, I don't know ;-), say: / # mount --help 'mount' mounts a file system. Usage: mount <fstype> <mp> [dev] [<moptions>] Hopefully, I will recall this great idea next time I get around to writing a usage message for my own program :-). Thanks for taking the time to look at the changes. Adam _______________________________________________ HelenOS-devel mailing list [email protected] http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
