Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restored old performance in ChartDataSet #3216

Merged
merged 1 commit into from
Jan 31, 2018
Merged

Restored old performance in ChartDataSet #3216

merged 1 commit into from
Jan 31, 2018

Conversation

jjatie
Copy link
Collaborator

@jjatie jjatie commented Jan 27, 2018

for #3166
Temporary fix for calcMinMax frequency.
Better implementation coming for Charts 4

Temporary fix for calcMinMax frequency.
Better implementation coming for Charts 4
@jjatie jjatie added this to the 3.1 milestone Jan 27, 2018
@jjatie jjatie requested a review from liuxuan30 January 27, 2018 00:04
@jjatie jjatie changed the title Restored old performance Restored old performance in ChartDataSet Jan 27, 2018
@codecov-io
Copy link

Codecov Report

Merging #3216 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3216      +/-   ##
==========================================
- Coverage   22.89%   22.87%   -0.02%     
==========================================
  Files         116      116              
  Lines       15609    15618       +9     
  Branches      272      272              
==========================================
  Hits         3573     3573              
- Misses      12000    12009       +9     
  Partials       36       36
Impacted Files Coverage Δ
...s/Data/Implementations/Standard/ChartDataSet.swift 36.09% <0%> (-0.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48bc049...937e8c2. Read the comment docs.

@liuxuan30
Copy link
Member

hmm.. what PR is before this invert? I don't remember you particularly have a performance enhance PR?

@liuxuan30 liuxuan30 added this to In progress in 3.1 release Jan 29, 2018
@jjatie
Copy link
Collaborator Author

jjatie commented Jan 30, 2018

I don't know which one was the problem. I think it was when I moved to the didSet. I think it's important we have a fix for now, I have a good idea on how to improve it in 4.0.

@liuxuan30
Copy link
Member

Weird I didn't catch this performance drop.

@liuxuan30
Copy link
Member

wait a minute. I checked it's #3000

    @objc open var values: [ChartDataEntry]
    {
        get
        {
            return _values
        }
        set
        {
            _values = newValue
            notifyDataSetChanged()
        }
    }

I didn't see didSet or set has any difference

@liuxuan30
Copy link
Member

liuxuan30 commented Jan 30, 2018

eh, I kind of start to recall. didSet will be called for Array type every time an element is added in or removed (and replace), is that correct? That's the drop.

However for set setter, add/remove does not trigger the setter. Damn, I should remember this all the time.

@liuxuan30
Copy link
Member

@jjatie do you think it's the right behavior for didSet on collection types? I will keep an eye on if swift evolution would address this. Kind of confusing.

@jjatie
Copy link
Collaborator Author

jjatie commented Jan 30, 2018

Yes, that’s the reason why. didSet is called whenever the underlying data mutates (excluding direct memory access). A computed variable doesn’t have underlying data, so it doesn’t make sense for it to be called on append/remove.

There’s a couple of major architectural changes I’m going to make over the next couple weeks. After that I’ll work in making performance better for 4.0

@liuxuan30
Copy link
Member

@jjatie take some rest.

@liuxuan30 liuxuan30 merged commit b1191bc into ChartsOrg:master Jan 31, 2018
3.1 release automation moved this from In progress to Done Jan 31, 2018
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Jan 31, 2018
* 'master' of https://github.com/danielgindi/Charts:
  Restored old performance (ChartsOrg#3216)
  Support other bundle than main MarkerView.viewFromXib() (ChartsOrg#3215)
  For ChartsOrg#2840. add dataIndex parameter in `highlightValue()` calls (ChartsOrg#2852)
  Balloon Marker indicates position of data (ChartsOrg#3181)
  bump Info.plist version
  BubbleChart uses correct colour for index now.

# Conflicts:
#	Source/Charts/Mark/BalloonMarker.swift
@jjatie jjatie deleted the restoring-old-performance branch February 1, 2018 00:10
kzyryanov pushed a commit to fishbrain/Charts that referenced this pull request Feb 20, 2018
Temporary fix for calcMinMax frequency.
Better implementation coming for Charts 4
guillermo-ag-95 added a commit to guillermo-ag-95/Project-Atlas that referenced this pull request Feb 22, 2018
Charts 3.0.5 (the current version) introduced a bug with caused a performance drop in real time charts (ChartsOrg/Charts#3166).

The patch will be released with the 3.1 version in the following weeks (ChartsOrg/Charts#3216)

As for now, updatesIntervalOn is set from 100Hz to 10Hz until the 3.1 is released.
kalmurzayev pushed a commit to kalmurzayev/Charts that referenced this pull request Feb 26, 2018
Temporary fix for calcMinMax frequency.
Better implementation coming for Charts 4
FreddyZeng added a commit to FreddyZeng/Charts that referenced this pull request Mar 14, 2018
* master: (55 commits)
  Refactors -[tableView:cellForRowAtIndexPath:] (ChartsOrg#3326)
  fix ChartsOrg#3311. Need one more key for iOS 11 camera roll saving
  revert a mistake, fix ChartsOrg#3299
  add pie chart unit tests (ChartsOrg#3297)
  ChartsOrg#3287: align Objc and Swift demos balloon marker
  update changelog
  Min and Max reset when clearing ChartDataSet (Fixes ChartsOrg#3260)
  Restored old performance (ChartsOrg#3216)
  Support other bundle than main MarkerView.viewFromXib() (ChartsOrg#3215)
  For ChartsOrg#2840. add dataIndex parameter in `highlightValue()` calls (ChartsOrg#2852)
  Balloon Marker indicates position of data (ChartsOrg#3181)
  bump Info.plist version
  Fixed a duplicated assignment compared with obj-c code. (ChartsOrg#3179)
  Updated README for 3.0.5 (ChartsOrg#3183)
  BubbleChart uses correct colour for index now.
  Added custom text alignment for noData
  Fixes the distance issue between the legend and the horizontal bar chart (Fixes ChartsOrg#2138) (ChartsOrg#2214)
  call setNeedsDisplay() here to trigger render noDataText (ChartsOrg#3198)
  添加定制TY的Mark
  添加修改过的Mark到工程中
  ...

# Conflicts:
#	ICXCharts.podspec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
3.1 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants