[karajorma]

The more adventurous FREDders amongst you will know that as a mission gets larger it takes longer and longer to close the Events Editor.

I got pissed off at a Diaspora mission taking over a minute to close the event editor, profiled FRED, found the cause and fixed it. I'll be adding this fix to SVN in a day or two but I wanted FREDders to check that it's not borking anything before I commit it (it really shouldn't be but given that the outcome if it does is ****ing up someone's mission, I figured it was worth double checking).

To test this, pick a mission. Save it under a different name. Delete an event or ship or message. Do the same in a regular FRED build. Compare the two missions using WinMerge or something similar. They should be identical apart from the save time.

For the coders, here's the patch. Yes, that's really all I did.

Code: [Select]

Index: parse/sexp.cpp
===================================================================
--- parse/sexp.cpp (revision 8680)
+++ parse/sexp.cpp (working copy)
@@ -25017,6 +25017,9 @@
 void update_sexp_references(const char *old_name, const char *new_name, int format)
 {
  int i;
+ if (!strcmp(old_name, new_name)) {
+ return;
+ }
 
  Assert(strlen(new_name) < TOKEN_LENGTH);
  for (i = 0; i < Num_sexp_nodes; i++)

It turns out that FRED checks that event names, message names, etc were checked using a recursive call to update_sexp_references(). Since this then checked every single SEXP node once for every single event in the game, as missions grow, the amount of calls made increases exponentially. Using a mission that took over a minute to close the event editor, I profiled it for 3 seconds and found it had made over 3 million calls to update_sexp_references. Meaning that it was probably making around 60 million calls in total when closing the event editor.

I have not fixed this behaviour.

What I fixed was it doing this when the old name and the new name were exactly the same. i.e the 99% of calls it never needed to make in the first place.


For the FREDders, here's the build.

[CommanderDJ]

* CommanderDJ applauds.

Brilliant. Will test this later today.

[Mongoose]

I have not fixed this behaviour.

What I fixed was it doing this when the old name and the new name were exactly the same. i.e the 99% of calls it never needed to make in the first place.

...that has to be the very definition of Nightmare Code.

[karajorma]

Well basically the problem never existed in Volition missions since they were so small. No one spotted that the code was doing unnecessary function calls as a result.

Hell, for all I know, I might actually be the first person to ever profile FRED. Usually we don't care about minor problems with FRED being slow.

[Fury]

Would you consider enabling "show ship models" by default because a lot of people don't seem to realize that the wireframe view slows down rendering to crawl?

[Spoon]

Incredible, its not even been a day since I made that not so serious complaint thread about this.

[Legate Damar]

Incredible, its not even been a day since I made that not so serious complaint thread about this.

You have lots of influence, it seems.

Will this be included in 3.6.14 final?

[karajorma]

If enough people test it and report back that it doesn't **** things up, maybe.

And actually it wasn't cause of Spoon. I added 30 events to a mission recently which pushed it over the edge from annoying to downright anger inducing. I happened to have set up profiling builds for Diaspora earlier this week for unrelated reasons so basically this was the first time that I've had time to fix the problem, a build already set up to do it with, and a mission that was displaying the behaviour at the same time.

[Spoon]

You have lots of influence, it seems.

lol no, this was purely coincidence

[jg18]

Nicely done and well-timed, too.

It's a nice shortcut that you know for a fact can save you a lot of unnecessary work since you profiled first.

How long did it take to close the events editor after you added the shortcut, btw?

A few thoughts:

(1) Is the growth really exponential? Whatever it is, it's obviously fast, and although I'm not (yet) familiar with the SEXP handling code, I'd think that the amount of checking would be
O(<number of SEXP nodes> x <number of events>).

(2) About the patch itself, you may want to adjust the patch as follows:

(a) Put the three lines you added above the

Code: [Select]

int i; line, to make it clear that that declaration fits with the rest of the code and not your new shortcut.

(b) Add a comment above your added lines, maybe

Code: [Select]

// If the name hasn't changed, there's nothing to update
(c) Move the assert to the top of the function, so that it checks the assert before evaluating your shortcut.

(d) Maybe even move the index variable i into the for loop, something couldn't do in C but that can done in C++, making the line read

Code: [Select]

for (int i = 0; i < Num_sexp_nodes; i++)
(I'm not backseat coding, I swear. )

EDIT: Slight corrections.

[karajorma]

I would move the declaration but it's a style thing. Everyone keeps using the uninitialised, right at the top of the function declaration style which means that I have to too.

We don't move the loop variables for instance cause we end up breaking compilation on VC6 whenever we do (VC6 has this rather stupid standard where you could then access i outside the loop. Meaning if anyone ever added a second loop to the function and re-used i, it would break compilation).

I didn't move the assertion cause if the names are the same, we really should be catching it earlier. The old name would have been broken for a long time.

Growth isn't really exponential but it is pretty big, permit me some hyperbole.

As for adding a comment. Never had a problem with that.

[Echelon9]

Yeh, looks good with those slight tweaks. Nice work karajorma.

[jg18]

I would move the declaration but it's a style thing. Everyone keeps using the uninitialised, right at the top of the function declaration style which means that I have to too.

I think in ANSI C (specifically C89), local variables have to be declared at the top of the function (or code block) in which they're used, which is probably why did things that way. C99 and C++ don't have that requirement, in which case it becomes a matter of style. As you can see, I like keeping related statements together, but that's just me.

[Cyborg17]

Thank you! 

[karajorma]

I think in ANSI C (specifically C89), local variables have to be declared at the top of the function (or code block) in which they're used, which is probably why did things that way. C99 and C++ don't have that requirement, in which case it becomes a matter of style. As you can see, I like keeping related statements together, but that's just me.

Oh trust me, I bloody hate the way things are done now. Increasing the scope of variables in that way is just going to cause unnecessary bugs. But that's the style. So unless we decide to change the style, it's not something one or two coders should just independently decide to change.

Especially when as I say, it can cause the code not to compile on other IDEs.

[chief1983]

Yes, please don't put the 'int i' inside a for loop declaration.

[Spoon]

Initial tests have shown no problems for me so far

[Spoon]

So after using it for some time now, I have not experienced any wierdness or troubles with this change in the event editor.
Not sure if any more feedback is required on this, since the last reply was from me, and it was a while ago but hey.

[mjn.mixael]

So after using it for some time now, I have not experienced any wierdness or troubles with this change in the event editor.
Not sure if any more feedback is required on this, since the last reply was from me, and it was a while ago but hey.

Quote'd for truth.

[karajorma]

Well it's been in trunk for a while now and no one has reported any issues with it. So hopefully the fix is good enough.