View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001521 | unreal | ircd | public | 2004-02-07 12:28 | 2004-05-26 21:38 |
| Reporter | AngryWolf | Assigned To | |||
| Priority | normal | Severity | feature | Reproducibility | N/A |
| Status | closed | Resolution | open | ||
| Product Version | 3.2-beta19 | ||||
| Summary | 0001521: /map code optimization | ||||
| Description | I'm sending you a patch that optimizes the code of m_map and dump_map, in addition it's also containing a little change to allow only displaying of server names that match the given mask (and their uplinks of course), except the first server on the list which remains to be always displayed. I hope you will like it. By the way, I'm not 100% sure if the code is bugless, but I believe it is. I've made a lot of tests. | ||||
| Attached Files | s_serv.c.diff (2,240 bytes) s_serv.c.diff2 (2,560 bytes) s_serv.c.diff3 (2,644 bytes) s_serv.c.diff.new (1,878 bytes) | ||||
| 3rd party modules | |||||
|
|
I've corrected s_serv.c.diff and uploaded s_serv.c.diff2, the newer patch only displays me.name if necessary. |
|
|
I'm sorry that I didn't send the right code at the first time. I've just found a way to make my code faster. (Uploaded s_serv.c.diff3.) edited on: 02-08-04 09:51 |
|
|
Perhaps I'm missing something, but how does adding an extra loop make this faster? |
|
|
OK, I'll give you a lot of details. When I first created this bug report, I only wanted to optimize the code of /map. Later it came into my mind that it would be helpful if parv[1] could be used to list only specific servers (that match a given mask), which was only possible if the uplinks were listed too. At the first time the code wasn't okay, because the local server was always displayed. So I uploaded diff2. What I did in diff3 is that I made the new algorythm faster by adding two extra conditions to break the loop and skip double work. That's all I'm talking about. [Edit: grammar corrections] edited on: 02-08-04 18:51 |
|
|
Well, I just "benchmarked" your latest patch.. and it's not faster at all, in fact it's a few usec slower in my tests... so the whole reason/bugtitle is misleading. If you care about speed you are also not looking at optimizing the right routines... currently /map in my 5-server-testnet@p450 takes 166usec (and yours 174).. that's completely reasonable for such a command. As for mask/parv[1] support... I don't think that would be really useful, but that's just my personal opinion. |
|
|
OK, forget it... |
|
|
But allow me a last note: I'm not talkin' about speed anymore, nor about optimization. I've lost my original ideas in them when I added parv[1] support, which can indeed only be useful on bigger networks. Of course this change makes the original code definitely slower, but new features normally don't increment speed. Eventually, if you decide not to keep it, that's okay, but please take a look at the current code of dump_map, because it doesn't look very fine to me. I'm sure I can write a faster code without incrementing the number of loops and conditions. Probably that's what I should have done from the first time. Attached s_serv.c.diff.new. Time results here (with 4 servers): 64 usecs (with the new code) againts 68 (with the original one). |
|
|
god... |
|
|
What? Does that mean good or bad? What am I doing wrong now? Why don't you delete this bugreport then? I don't care anymore. |
|
|
> "but new features normally don't increment speed" This can be ambiguous. Better said "new features normally don't make the program faster". But why do I explain things that you know better... |
|
|
1. It does not do [or did] what it claims to be (code optimization) [*] 2. Code optimization is not needed in this case [*] 3. The "what is gained by this" vs "risks" score is very low 4. The fact that you posted 3 patches (and now 4) is also not helping 5. I didn't really look into it detailed coz I found it difficult to read the patch and didn't want to waste too much time. (see next) 6. We are preparing for next release, so this won't make it. 7. As said earlier [2], code optimization is not really needed.. if you then come with a 4th patch [!] with a 4 usec gain and get rid of the only possible value added feature (the parv[1] stuff).. that's almost like a joke! [* = as mentioned earlier] [3] being the main point, all the other points are of course not helping either. Reason I didn't close the bugreport is that this could be considered as some kind of code cleanup. And now I wasted 10 minutes again on writing this bugnote #@$&@# (but that's my own fault ;p). |
|
|
On a sidenote, this is not personal.. if you were called BlahTehDuck I would have reacted in the same manner (or possibly even stronger :P)... The thing we are looking at all the time is: "what do we gain by this?" vs "how much time does it take to do / what is the risk involved / etc..". |
|
|
Could you perhaps explain why exactly the mask feature is useful? |
|
|
Syzop: apparently, the first two patches should have already been deleted. All this stuff I made here are, of course, not important, hence take your time in releasing beta20, no hurry at all. I've realized I still have to learn things about real "optimization", and I'm sorry that I was only wasting your time. codemastr: I have only one reason: on a big network if you don't want to see all the representation of the network hierarchy, but want to know how a specific server is routed (or more) from the viewpoint of the local server, you can save some lines. On a sidenote, if the goal is to see the route between two servers, using /map from any servers of the network would also be useful to me. If you find this a weak reason, no problem. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2004-02-07 12:28 | AngryWolf | New Issue | |
| 2004-02-07 12:28 | AngryWolf | File Added: s_serv.c.diff | |
| 2004-02-07 13:45 | AngryWolf | File Added: s_serv.c.diff2 | |
| 2004-02-07 13:47 | AngryWolf | Note Added: 0004899 | |
| 2004-02-08 09:45 | AngryWolf | File Added: s_serv.c.diff3 | |
| 2004-02-08 09:48 | AngryWolf | Note Added: 0004901 | |
| 2004-02-08 09:51 | AngryWolf | Note Edited: 0004901 | |
| 2004-02-08 17:22 |
|
Note Added: 0004902 | |
| 2004-02-08 18:30 | AngryWolf | Note Added: 0004903 | |
| 2004-02-08 18:33 | AngryWolf | Note Edited: 0004903 | |
| 2004-02-08 18:51 | AngryWolf | Note Edited: 0004903 | |
| 2004-02-08 19:55 | syzop | Note Added: 0004904 | |
| 2004-02-08 20:05 | AngryWolf | Note Added: 0004905 | |
| 2004-02-08 22:28 | AngryWolf | Note Added: 0004906 | |
| 2004-02-08 22:29 | AngryWolf | File Added: s_serv.c.diff.new | |
| 2004-02-08 22:31 | syzop | Note Added: 0004907 | |
| 2004-02-08 22:40 | AngryWolf | Note Added: 0004908 | |
| 2004-02-08 22:52 | AngryWolf | Note Added: 0004910 | |
| 2004-02-08 23:13 | syzop | Note Added: 0004911 | |
| 2004-02-08 23:17 | syzop | Note Added: 0004912 | |
| 2004-02-09 04:12 |
|
Note Added: 0004913 | |
| 2004-02-09 16:31 | AngryWolf | Note Added: 0004917 | |
| 2004-05-26 21:38 | syzop | Status | new => closed |
