Memory leak deterioration in performance in working with weight in rs232 Win-CE
-
hi, i have Windows-Mobile program that work with weight Connecting through rs232. its work excellent - If the program works in a period of time, it starts to slow down and even get stuck at some point. my code: Hide Expand Copy Code port = new SerialPort("COM3", 9600, Parity.None, 8, StopBits.One); port.DataReceived += new System.IO.Ports.SerialDataReceivedEventHandler(Recepcion); private SerialPort port; StringBuilder SB; private void Recepcion(object sender, System.IO.Ports.SerialDataReceivedEventArgs e) { try { SB = new StringBuilder(1000); Application.DoEvents(); System.Threading.Thread.Sleep(122); SB.Append(port.ReadExisting()); port.DiscardInBuffer(); this.Invoke(new EventHandler(Actualizar)); } catch { } } string MOMO1, MOMO2; string[] WI; string ALL; private void Actualizar(object s, EventArgs e) { ALL = SB.ToString().Trim(); WI = ALL.Split(','); ALL = WI[2].ToString().Trim(); MOMO1 = ALL.Replace("+", "").Replace("g", "").Replace("ST", "").Replace("GS", "").Replace("US", ""); if (MOMO1 != "") { MOMO2 = MOMO1; } lblMSG.Font = new Font("Ariel", 48, FontStyle.Bold); lblMSG.Text = MOMO2; Check_Weight(); GC.Collect(); // <-- is it OK ? } Can anyone advise me why and how to solve it ?
-
hi, i have Windows-Mobile program that work with weight Connecting through rs232. its work excellent - If the program works in a period of time, it starts to slow down and even get stuck at some point. my code: Hide Expand Copy Code port = new SerialPort("COM3", 9600, Parity.None, 8, StopBits.One); port.DataReceived += new System.IO.Ports.SerialDataReceivedEventHandler(Recepcion); private SerialPort port; StringBuilder SB; private void Recepcion(object sender, System.IO.Ports.SerialDataReceivedEventArgs e) { try { SB = new StringBuilder(1000); Application.DoEvents(); System.Threading.Thread.Sleep(122); SB.Append(port.ReadExisting()); port.DiscardInBuffer(); this.Invoke(new EventHandler(Actualizar)); } catch { } } string MOMO1, MOMO2; string[] WI; string ALL; private void Actualizar(object s, EventArgs e) { ALL = SB.ToString().Trim(); WI = ALL.Split(','); ALL = WI[2].ToString().Trim(); MOMO1 = ALL.Replace("+", "").Replace("g", "").Replace("ST", "").Replace("GS", "").Replace("US", ""); if (MOMO1 != "") { MOMO2 = MOMO1; } lblMSG.Font = new Font("Ariel", 48, FontStyle.Bold); lblMSG.Text = MOMO2; Check_Weight(); GC.Collect(); // <-- is it OK ? } Can anyone advise me why and how to solve it ?
For starters, you should never have to call in the garbage collector yourself: it only needs to be used when you start to run out of memory. Second, that's really rather nasty code. The DataReceived event is started on a non-UI thread, as you know (or you wouldn't be Invoking the UI), but you are changing / using the value of SB on two threads, with no checking or locking to make sure that there isn't a problem. And you Invoke the UI thread even if there is no data to play with - and DataReceived can be fired for each character received, even if you have already removed them from the buffer. You are also creating a new Font object (which is identical to the last) potentially for each character you receive, without any attempt to dispose of them. The GC will not do that, so Graphics Handles are being used up at a frightening rate - and these do not trigger the GC to do anything. And there is a spurious ToString in there: Split returns an array of strings, so converting its constituent parts to a string is unnecessary. Personally? I'd have a background task processing the data with locking to prevent problems, and only invoke the bits which update the actual UI elements. I'd also use the same Font object for the label, and not change it at all. And Application.DoEvents is a definite no-no! I'd probably not use Split at all - it's not particularly efficient since it creates a lot of memory you aren't using. I'd look at IndexOf and SubString instead to "pull out" just the bit you are interested in. Even if I did use Split, you really, really need to check the number of array elements it returns: The DataReceived event doesn't wait for the end of your message before it "kicks in"... [edit]Typos[/edit]
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
-
For starters, you should never have to call in the garbage collector yourself: it only needs to be used when you start to run out of memory. Second, that's really rather nasty code. The DataReceived event is started on a non-UI thread, as you know (or you wouldn't be Invoking the UI), but you are changing / using the value of SB on two threads, with no checking or locking to make sure that there isn't a problem. And you Invoke the UI thread even if there is no data to play with - and DataReceived can be fired for each character received, even if you have already removed them from the buffer. You are also creating a new Font object (which is identical to the last) potentially for each character you receive, without any attempt to dispose of them. The GC will not do that, so Graphics Handles are being used up at a frightening rate - and these do not trigger the GC to do anything. And there is a spurious ToString in there: Split returns an array of strings, so converting its constituent parts to a string is unnecessary. Personally? I'd have a background task processing the data with locking to prevent problems, and only invoke the bits which update the actual UI elements. I'd also use the same Font object for the label, and not change it at all. And Application.DoEvents is a definite no-no! I'd probably not use Split at all - it's not particularly efficient since it creates a lot of memory you aren't using. I'd look at IndexOf and SubString instead to "pull out" just the bit you are interested in. Even if I did use Split, you really, really need to check the number of array elements it returns: The DataReceived event doesn't wait for the end of your message before it "kicks in"... [edit]Typos[/edit]
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
-
Many thanks for the detailed answer ! If this is not hard, I'd be happy if you can change things problematic in this code.
OG has given you a detailed answer of the issues in your code, it shouldn't be too much trouble for you to convert it based off his reply. You might want to do this for yourself.
-
Many thanks for the detailed answer ! If this is not hard, I'd be happy if you can change things problematic in this code.
Just to add to what Pete said, I couldn't if I wanted to: I don't have access to the equipment you are connecting to, or any idea what the data looks like, or what's important about it - so I couldn't test any changes I did make!
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
-
For starters, you should never have to call in the garbage collector yourself: it only needs to be used when you start to run out of memory. Second, that's really rather nasty code. The DataReceived event is started on a non-UI thread, as you know (or you wouldn't be Invoking the UI), but you are changing / using the value of SB on two threads, with no checking or locking to make sure that there isn't a problem. And you Invoke the UI thread even if there is no data to play with - and DataReceived can be fired for each character received, even if you have already removed them from the buffer. You are also creating a new Font object (which is identical to the last) potentially for each character you receive, without any attempt to dispose of them. The GC will not do that, so Graphics Handles are being used up at a frightening rate - and these do not trigger the GC to do anything. And there is a spurious ToString in there: Split returns an array of strings, so converting its constituent parts to a string is unnecessary. Personally? I'd have a background task processing the data with locking to prevent problems, and only invoke the bits which update the actual UI elements. I'd also use the same Font object for the label, and not change it at all. And Application.DoEvents is a definite no-no! I'd probably not use Split at all - it's not particularly efficient since it creates a lot of memory you aren't using. I'd look at IndexOf and SubString instead to "pull out" just the bit you are interested in. Even if I did use Split, you really, really need to check the number of array elements it returns: The DataReceived event doesn't wait for the end of your message before it "kicks in"... [edit]Typos[/edit]
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
thanks for the help, i change my code to: private SerialPort port; StringBuilder SB; private void Recepcion(object sender, System.IO.Ports.SerialDataReceivedEventArgs e) { try { SB = new StringBuilder(1000); SB.Append(port.ReadLine()); port.DiscardInBuffer(); this.Invoke(new EventHandler(Actualizar)); } catch { } } but still same problem..... my program fonts change alone for no reason.....:confused::confused:
-
thanks for the help, i change my code to: private SerialPort port; StringBuilder SB; private void Recepcion(object sender, System.IO.Ports.SerialDataReceivedEventArgs e) { try { SB = new StringBuilder(1000); SB.Append(port.ReadLine()); port.DiscardInBuffer(); this.Invoke(new EventHandler(Actualizar)); } catch { } } but still same problem..... my program fonts change alone for no reason.....:confused::confused:
All you have done it take out the DoEvents and the Thread.Sleep. You have done nothing about the problems I told you about. And you are surprised that your code has the same fault? Are you just guessing here, or have you designed this properly?
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
-
All you have done it take out the DoEvents and the Thread.Sleep. You have done nothing about the problems I told you about. And you are surprised that your code has the same fault? Are you just guessing here, or have you designed this properly?
Bad command or file name. Bad, bad command! Sit! Stay! Staaaay...
